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

Removes array delimiters while parsing parameters #292

Merged
merged 1 commit into from
Sep 8, 2017

Conversation

Behery
Copy link
Member

@Behery Behery commented Aug 30, 2017

Also refactors the way the parameters are obtained. Fixes #291

@nuclearsandwich
Copy link
Member

The Python changes look good. I like the refactor: one local function is much easier to reason about and maintain than three list comprehensions. It's also good that this only strips array brackets rather than truncating the first and last character and implicitly assuming array brackets are present.

There's a part of me that wants to find a way to perform these string manipulations all in one pass using a regular expression capture but that's an indulgence worth ignoring unless this actually becomes a performance problem.

There are no tests for this project but can you provide a minimal example that is broken with the latest release and fixed by this PR?

Thanks!

@nuclearsandwich
Copy link
Member

Oh, forgot to mention, the changes that this complements / fixes were introduced in #252

@Behery
Copy link
Member Author

Behery commented Sep 6, 2017

Using the following launch file would cause it to break in the current release. The rosbridge_server will try to match service requests against 'o_something', and 'do_something_els' and will not be able to call the correct services.

<launch>
    <node name="my_ros_bridge_node" pkg="rosbridge_server" type="rosbridge_websocket.py" cwd="node" output="screen" respawn="true">
        <param name='services_glob' value='do_things, do_something_else'/>
    </node>
</launch>

@nuclearsandwich
Copy link
Member

@jihoonl do we have a merge strategy for pull requests beyond "merge when ready"? Is there anything I should do to merge this, like changelog updates or other notes?

@jihoonl
Copy link
Member

jihoonl commented Sep 8, 2017

Not really. As soon as you approve, feel free to merge. I am merging it.

@jihoonl jihoonl merged commit 31fbad3 into RobotWebTools:develop Sep 8, 2017
@nuclearsandwich
Copy link
Member

Great! Thanks Behery for the contribution 🌈

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants