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

move launch files into launch.legacy namespace #73

Merged
merged 2 commits into from
Jun 5, 2018

Conversation

wjwwood
Copy link
Member

@wjwwood wjwwood commented Jun 4, 2018

In preparation for opening pull requests for the new launch API I've been working on, this pull request (and the connected ones) will simply move the existing launch system into a sub package (called legacy) and updates anything using launch to also use this new API.

That way I can introduce the new API in smaller steps without first having to have it completely replace the existing tool and work in all cases.

This is the end goal, but there are a few things in launch_testing and a few of the output handlers I still need to port before I can start doing that. Plus this gives us something to compare and fallback to as we start using/tryout the new tool.

@wjwwood wjwwood force-pushed the refactor_launch_into_launch_legacy branch from cf89281 to 71d8ef9 Compare June 5, 2018 19:15
@wjwwood wjwwood added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Jun 5, 2018
@wjwwood
Copy link
Member Author

wjwwood commented Jun 5, 2018

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status (this is a "performance" test failure, and I think it's unrelated as nothing this test uses nor rclpy have been changed in these pr's)

@wjwwood
Copy link
Member Author

wjwwood commented Jun 5, 2018

This is ready for review.

@dirk-thomas
Copy link
Member

Would the history be preserved if the move would be done in two steps: one commit just moving the file, another commit modifying the files? If yes, I would advocate for that.

@wjwwood
Copy link
Member Author

wjwwood commented Jun 5, 2018

No, it wouldn't appear that way on Github, but either way you do it, it is available via github's blame history and on the command line with git log --follow.

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.

Since CI passed this LGTM.

@wjwwood
Copy link
Member Author

wjwwood commented Jun 5, 2018

Btw, the feature was requested but never went anywhere, but there is a chrome extension to help with it:

https://stackoverflow.com/questions/5646174/how-to-make-github-follow-directory-history-after-renames

@wjwwood
Copy link
Member Author

wjwwood commented Jun 5, 2018

I'll take the positive review to apply to the other pr's too since they're so automatically derivative. Thanks!

@wjwwood wjwwood merged commit fb6cbe4 into master Jun 5, 2018
@wjwwood wjwwood deleted the refactor_launch_into_launch_legacy branch June 5, 2018 21:33
@dirk-thomas
Copy link
Member

Was there a reason why the launch_testing module wasn't moved into a legacy namespace too? I am considering doing that now to "make room" for new testing logic as well as keep the API "symmetric".

@wjwwood
Copy link
Member Author

wjwwood commented Aug 24, 2018

No reason that’s fine for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in review Waiting for review (Kanban column)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants