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

Bug fix for fields in service calls with different names. #347

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Bug fix for fields in service calls with different names. #347

wants to merge 6 commits into from

Conversation

tompe17
Copy link

@tompe17 tompe17 commented Mar 21, 2022

This is my fix for the issue in #285

@fujitatomoya
Copy link
Collaborator

@tompe17 could you address DCO error https://github.com/ros2/ros1_bridge/pull/347/checks?check_run_id=5623872538 ?

@tompe17 tompe17 force-pushed the fieldnamebug branch 2 times, most recently from b9c8200 to f456625 Compare March 22, 2022 08:08
gbiggs
gbiggs requested changes Mar 25, 2022
View reviewed changes
ros1_bridge/__init__.py Outdated Show resolved Hide resolved
@tompe17 tompe17 force-pushed the fieldnamebug branch 2 times, most recently from c49c0e5 to b6abc94 Compare March 25, 2022 11:55
gbiggs and others added 4 commits March 25, 2022 12:56
@gbiggs
Update package maintainers (ros2#335)
374335b 
* Update package maintainers

Signed-off-by: Geoffrey Biggs <[email protected]>
Signed-off-by: Tommy Persson <[email protected]>
@Blast545
Fix cpplint error (ros2#341)
318ccfe 
* Fix cpplint error

Signed-off-by: Jorge Perez <[email protected]>
Signed-off-by: Tommy Persson <[email protected]>
@nirwester
Parametrizing service execution timeout (ros2#340)
4170a04 
* Parametrizing service execution timeout

Signed-off-by: Marco Bassa <[email protected]>
Signed-off-by: Tommy Persson <[email protected]>
Bug fix for fields in service calls with different names.
34c1b3c 
Signed-off-by: Tommy Persson <@[email protected]>
Signed-off-by: Tommy Persson <[email protected]>
@gbiggs
Copy link
Member

gbiggs commented Mar 25, 2022

CI:

  • Linux Build Status

fujitatomoya
fujitatomoya reviewed Mar 25, 2022
View reviewed changes
Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

just curious, why are there unrelated changes which are already in master branch?

@tompe17
Copy link
Author

tompe17 commented Apr 10, 2022

Do I need to do anything more with this or will this be merged? It would very good if this functionality could work in the next release of ROS2 since we will depend on it until DJI releases it ROS SDK for ROS2 if that ever will happen.

@gbiggs
Copy link
Member

gbiggs commented Apr 10, 2022

@tompe17 You have been asked a question by @fujitatomoya. I'm waiting for your reply before merging this.

@tompe17
Copy link
Author

tompe17 commented Apr 19, 2022

Concerning unrelated changed since I have not the faintest idea about why that is I could not answer it. I can only describe what I did:

  • I forked the repo
  • I made a branch and did the changes
  • I made the pull request
  • There was the issue with a signature so I followed the instructions to fix that. Some day after that I for a messaged that the email address was invalid since it by mistake started with @.
  • So i tried to fix that and repeted the original fix that was suggested after fixing my buggy email address I had in a config file.
  • I tried that a couple of times since it did not seem to help. The I saw that @gbiggs hade approves something so I thought everything was OK.

I am not familiar enough with github and pull requests to understand if what I did have caused some problems or not.

@gbiggs
Copy link
Member

gbiggs commented May 5, 2022

@fujitatomoya Could you be specific about which changes you think are unrelated?

@methylDragon
Copy link
Contributor

@gbiggs

My suspicion is these:
image

Suggestion: Rebase the branch on master, which should remove a lot of the extra commits, it'll also fix the conflicts since the creation of this PR.

@JonSmet JonSmet mentioned this pull request Jun 6, 2022
Merged
@gbiggs
Copy link
Member

gbiggs commented Jun 29, 2022

@tompe17 Can you please rebase your PR?

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

@fujitatomoya fujitatomoya fujitatomoya left review comments

@gbiggs gbiggs gbiggs approved these changes

Assignees

@gbiggs gbiggs

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@tompe17 @fujitatomoya @gbiggs @methylDragon @Blast545 @nirwester