Skip to content

Update parameters typing to leverage recursive types #443

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

Open
christophebedard opened this issue Feb 24, 2025 · 4 comments
Open

Update parameters typing to leverage recursive types #443

christophebedard opened this issue Feb 24, 2025 · 4 comments
Labels
help wanted Extra attention is needed

Comments

@christophebedard
Copy link
Member

Looks like recursive types are supported by mypy now: python/mypy#731.

  1. There's a TODO here:
    # TODO(sloretz) Recursive type when mypy supports them python/mypy#731
    _SomeParametersDict = Mapping[SomeParameterName, Any]
    SomeParametersDict = Mapping[SomeParameterName, Union[SomeParameterValue, _SomeParametersDict]]
    • Update the code to leverage recursive types and remove the TODO.
  2. There's also a comment below that mentions "(flattened to avoid having a recursive type)," which could therefore probably also be updated, although I'm not quite sure what to do there:
    # Normalized (flattened to avoid having a recursive type) parameter dict
    ParametersDict = Dict[ParameterName, ParameterValue]
    • I'm not exactly sure what it refers to. However, for this one, we could probably remove "(flattened to avoid having a recursive type)" from the comment and leave the code as-is.
  3. Finally, there are a few references to "making/appeasing mypy" in launch_ros/utilities/normalize_parameters.py. Perhaps that could be revisited to see if we can simplify it. # type: comments could also be updated to the more modern type annotations.
@christophebedard christophebedard added the help wanted Extra attention is needed label Feb 24, 2025
@InvincibleRMC
Copy link

InvincibleRMC commented Feb 24, 2025

rhel mypy's version is 0.971. So Using the recursive types would fail since version >= 1.7 is needed.

@christophebedard christophebedard removed the help wanted Extra attention is needed label Feb 24, 2025
@christophebedard
Copy link
Member Author

Good point! But I believe it's using 0.971.

I guess we'll have to wait a little while more then, since I don't think we'll switch to using pip to install a newer mypy version. I'll just leave this open for now.

@alsora alsora added the help wanted Extra attention is needed label Mar 6, 2025
@alsora
Copy link

alsora commented Mar 6, 2025

Hi @christophebedard, we discussed this issue in the waffle meeting and we decided to re-add "help wanted" label.
Do you have any thoughts on that?

@christophebedard
Copy link
Member Author

I removed it because this issue wasn't immediately actionable based on my quick investigation, but, sure, that works for me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants