Fix quoted strings for new flake8-quote check. #42
Conversation
|
CI run is in ros2/build_farmer#179 (comment) |
clalancette
force-pushed
the
flake8-quote-fixes
branch
from
38ae9c1
to
99eb52c
Compare
| @@ -140,7 +140,7 @@ for member in message.structure.members: | |||
| @[end for]@ | |||
| @[for member in message.structure.members]@ | |||
| @[ if member.has_annotation('default')]@ | |||
| '@(member.name.upper())__DEFAULT': @(value_to_py(member.type, member.get_annotation_value('default')['value'])), | |||
| '@(member.name.upper())__DEFAULT': @(value_to_py(member.type, member.get_annotation_value('default')['value'])), # noqa: Q003 | |||
There was a problem hiding this comment.
Instead of applying the warning suppression to any kind of content I would suggest to update the primitive_value_to_py() function instead to use the proper quoting.
Same below.
There was a problem hiding this comment.
The problem is figuring out the proper quoting. To do that, we actually have to walk the entire string and see whether there are any embedded single or double quotes, and then use the opposite. We'll also need to handle escaped quotes properly. In some cases, we won't be able to figure out one, so we'll still have to arbitrarily choose one.
It just doesn't seem worth it either for development time or for runtime. What does the check really buy us?
There was a problem hiding this comment.
That the generated code follows our code style guidelines.
There was a problem hiding this comment.
But that's just not a good enough reason, particularly for generated code, in my opinion. All of this stuff has a cost in terms of time and complexity, and doing busywork like this just doesn't seem to be a good use of either.
There was a problem hiding this comment.
I simply implemented the logic in a couple of minutes: see #43
clalancette
added
in review
Note that for generated code, we unconditionally disable the flake8-quote check Q003 (Change outer quotes to avoid escaping inner quotes). For non-string types, this will have no effect. For string types, it could be fairly expensive to scan strings for single or double quotes. The expense just doesn't seem worth it and the resulting return value won't be exactly what the user typed into their .msg file. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
clalancette
force-pushed
the
flake8-quote-fixes
branch
from
99eb52c
to
031aeb0
Compare
|
All right, with #43 going in, we just need the changes to the test strings here, and we'll get rid of all of the flake8 warnings. So this is ready for another review. |
Note that for generated code, we unconditionally disable the
flake8-quote check Q003 (Change outer quotes to avoid escaping
inner quotes). For non-string types, this will have no effect.
For string types, it could be fairly expensive to scan strings
for single or double quotes. The expense just doesn't seem
worth it and the resulting return value won't be exactly what
the user typed into their .msg file.
Signed-off-by: Chris Lalancette clalancette@openrobotics.org
Connects to ros2/build_farmer#179