Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RewrittenYaml does not parse integers keys #3483

Closed
mrmara opened this issue Mar 16, 2023 · 2 comments
Closed

RewrittenYaml does not parse integers keys #3483

mrmara opened this issue Mar 16, 2023 · 2 comments
Labels
bug Something isn't working

Comments

@mrmara
Copy link
Contributor

mrmara commented Mar 16, 2023

Bug report

  • Operating System:
    • Ubuntu 22.04
  • ROS2 Version:
    • Humble binaries
  • Version or commit hash:
    -Nav2 from source: 30a14090847be6f51f6cd60cbc400a0e4ac38b44
  • DDS implementation:
    • Cyclone

Steps to reproduce issue

RewrittenYaml method cannot properly parse yaml files with integers as key, e.g.:

node_number: 3
       0:
          node_name: "NODE1"
          node_frequency: 50
          diagnostic_message_enable: true
       1:
          node_name: "NODE2"
          node_frequency: 20
          diagnostic_message_enable: true
       2:
           node_name: "NODE3"
           node_frequency: 20
           diagnostic_message_enable: true

Expected behavior

RewrittenYaml() properly parse file and does not throw error

Actual behavior

Following error in thrown:

  File "/home/antoniomarangi/ws/install/nav2_common/local/lib/python3.10/dist-packages/nav2_common/launch/rewritten_yaml.py", line 89, in perform
    self.substitute_params(data, param_rewrites)
  File "/home/antoniomarangi/ws/install/nav2_common/local/lib/python3.10/dist-packages/nav2_common/launch/rewritten_yaml.py", line 116, in substitute_params
    yaml_paths = self.pathify(yaml)
  File "/home/antoniomarangi/ws/install/nav2_common/local/lib/python3.10/dist-packages/nav2_common/launch/rewritten_yaml.py", line 156, in pathify
    self.pathify(d, "", paths, joinchar=joinchar)
  File "/home/antoniomarangi/ws/install/nav2_common/local/lib/python3.10/dist-packages/nav2_common/launch/rewritten_yaml.py", line 164, in pathify
    self.pathify(v, pn + k, paths, joinchar=joinchar)
  File "/home/antoniomarangi/ws/install/nav2_common/local/lib/python3.10/dist-packages/nav2_common/launch/rewritten_yaml.py", line 164, in pathify
    self.pathify(v, pn + k, paths, joinchar=joinchar)
  File "/home/antoniomarangi/ws/install/nav2_common/local/lib/python3.10/dist-packages/nav2_common/launch/rewritten_yaml.py", line 164, in pathify
    self.pathify(v, pn + k, paths, joinchar=joinchar)
  [Previous line repeated 3 more times]
TypeError: can only concatenate str (not "int") to str

[ERROR] [launch]: Caught exception in launch (see debug for traceback): can only concatenate str (not "int") to str

Feature description

I do not know if using integers as key is a best practice or not, but is allowed by yaml format.
Moreover, it is really practical when the number of certain parameters is known a priori but can frequently change, e.g.:

// Declare parameters
get_parameter("node_number", node_number);
  for (int i = 0; i < node_number; i++)
  {
    std::string node_item = "nodes." + std::to_string(i);
		this->declare_parameter(node_item + ".node_name", "no_name"); 
    this->declare_parameter(node_item + ".node_frequency", 10);
    this->declare_parameter(node_item + ".diagnostic_message_enable", false);
}
  // Get parameters and store them
  for (int i = 0; i < node_number; i++)
  {
    std::string node_item = "nodes." + std::to_string(i);
    NodeParams np;
    get_parameter<std::string>(node_item + ".node_name", np.node_name);
    get_parameter<int>(node_item + ".node_frequency", np.node_frequency);
    get_parameter<bool>(node_item + ".diagnostic_message_enable", np.diagnostic_message_enable);
}

Implementation considerations

A fix could consist in str casting inside RewrittenYaml.pathify()
In particulat cast to str pn and k as long with idx

def pathify(self, d, p=None, paths=None, joinchar='.'):
        if p is None:
            paths = {}
            self.pathify(d, "", paths, joinchar=joinchar)
            return paths
        pn = p
        if p != "":
            pn += joinchar
        if isinstance(d, dict):
            for k in d:
                v = d[k]
-------->    self.pathify(v, str(pn) + str(k), paths, joinchar=joinchar) <---------------------
        elif isinstance(d, list):
            for idx, e in enumerate(d):
                self.pathify(e, pn + str(idx), paths, joinchar=joinchar)
        else:
            paths[p] = d
@SteveMacenski
Copy link
Member

Hi!

I would say that ints as keys are probably not a great solutions in my personal opinion but there’s nothing wrong with it. What you suggest is reasonable, can you open a PR? Happy to merge the change!

@SteveMacenski SteveMacenski added the bug Something isn't working label Mar 16, 2023
@mrmara
Copy link
Contributor Author

mrmara commented Mar 16, 2023

I will test the solution to make sure everything is fine and prepare the PR.

Thanks for the support

@mrmara mrmara changed the title RewrittenYaml does not pasre integers keys RewrittenYaml does not parse integers keys Mar 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants