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

Make import_message_from_namespaced_type operate on NamespacedType #74

Merged
merged 3 commits into from
Jul 26, 2019

Conversation

mikaelarguedas
Copy link
Member

@mikaelarguedas mikaelarguedas commented Jul 20, 2019

Fixes #70

Instead of changing the type specification, which would either require changing the function name (breaking API) or have the name and the behavior mismatch (current state), this modifies the function to operate on NamespacedType instead.
This will change behavior for consumers of the function.
If this is not desired, the less disruptive alternative is to change the function specification without changing the function name or the behavior.

Added a simple test to cover the uses of the function used in this package.

@dirk-thomas
Copy link
Member

For backward compatibility it would be nice to keep the previous usage pattern working.

We don't want to add a new function with a different name (the current name is just "right"). So how about the function checks if the passed message_type argument isn't of type NamespacedType and if that is the case and the variable has a value_type attribute then do message_type = message_type.value_type?

@mikaelarguedas
Copy link
Member Author

So how about the function checks if the passed message_type argument isn't of type NamespacedType and if that is the case and the variable has a value_type attribute then do message_type = message_type.value_type?

I thought about this originally, the uncertainty I had was: "what would be the type of the argument in the signature?"

  • Keep just NamespacedType even if the function can work with other types
  • Use a specification matching more closely the accepted types: e.g. Union[NamespacedType, AbstractNestedType] in this case the change could simply be (though less permissive than accepting any type that has a attribute value_type):
-def import_message_from_namespaced_type(message_type: NamespacedType) -> Any:
+def import_message_from_namespaced_type(
+        message_type: Union[NamespacedType, AbstractNestedType]) -> Any:
+    if isinstance(message_type, AbstractNestedType):
+        message_type = message_type.value_type
  • Something else?

@dirk-thomas
Copy link
Member

I would choose:

Keep just NamespacedType even if the function can work with other types

I would consider the special case to keep current usage working as deprecated. Maybe add a deprecation in the case where accessing value_type so that existing usage can be updated and the logic removed in the future.

@mikaelarguedas
Copy link
Member Author

Sounds good, done in 0002388

@dirk-thomas
Copy link
Member

Please rebase against the latest state on the target branch in order to run CI.

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
added a deprecation warning and updated the test

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
@dirk-thomas
Copy link
Member

dirk-thomas commented Jul 26, 2019

Thanks for the patch: Build Status

I updated your description to automatically close the referenced issue.

@dirk-thomas dirk-thomas added the bug Something isn't working label Jul 26, 2019
@dirk-thomas dirk-thomas merged commit f4599c7 into ros2:master Jul 26, 2019
@mikaelarguedas mikaelarguedas deleted the fix_import_namespaced_type branch July 26, 2019 19:17
jacobperron pushed a commit to ros2/rosidl_runtime_py that referenced this pull request Sep 19, 2019
…os2/rosidl_python#74)

* make import_message_from_namespaced_type operate on NamespacedTypes

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>

* basic test for import_messages_from_namespaced_type

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>

* restore support for types with attribute value_type

added a deprecation warning and updated the test

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
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

Successfully merging this pull request may close these issues.

Erroneous mypy specifications
2 participants