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.

Sign up for GitHub

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

Jump to bottom

Update dynamic.py #80

Closed
wants to merge 1 commit into from
Closed

Update dynamic.py #80

wants to merge 1 commit into from

Conversation

robbeofficial
Copy link

allow non-ascii chars in MSG files

@robbeofficial
Update dynamic.py
75720ce 
allow non-ascii chars in MSG files
dirk-thomas
dirk-thomas reviewed Mar 28, 2017
View reviewed changes
Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add a unit test for this use case. This will also be used as a reproducible example.

src/genpy/dynamic.py
@@ -165,7 +165,8 @@ def generate_dynamic(core_type, msg_cat):

# write the entire text to a file and import it (it will get deleted when tmp_dir goes - above)
tmp_file = tempfile.NamedTemporaryFile(suffix=".py",dir=tmp_dir,delete=False)
tmp_file.file.write(full_text.encode())
#tmp_file.file.write(full_text.encode())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this line since the next line replaces it. The git history will be sufficient.

@dirk-thomas
Copy link
Member

We are aiming for a new round of releases early next week (Mon or Tue). It would be great f this would be ready before in order to get released.

@moooeeeep moooeeeep mentioned this pull request Jul 9, 2018
Closed
@moooeeeep
Copy link
Contributor

moooeeeep commented Jul 12, 2018 •
edited
Loading

You can add something like this to test_genpy_dynamic.py to have a unit test for this use case:

try:
    msgs = generate_dynamic("gd_msgs/MyAngle", "float32 angle # \xc2\xb0\n")
except UnicodeDecodeError:
    assert False, "Can't handle UTF-8 in comments"

@dirk-thomas
Copy link
Member

@moooeeeep Please provide a pull request for proposed code changes.

@peteflorence
Copy link

@dirk-thomas can I open a new PR and get this merged?
This just burned me so bad having a unicode character in a ROS msg comment :), see: nalt/wsg50-ros-pkg#10

or @robbeofficial can you push this through?

@dirk-thomas
Copy link
Member

can I open a new PR

Sure.

get this merged?

This will depend on another review but if it works as expected in Python 2 and 3 as well as doesn't break anything then it can be merged.

@dirk-thomas
Copy link
Member

Closing due to no activity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dirk-thomas dirk-thomas dirk-thomas left review comments

Assignees
No one assigned
Labels
requires-changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@robbeofficial @dirk-thomas @moooeeeep @peteflorence