-
Notifications
You must be signed in to change notification settings - Fork 27
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
Increase test coverage ament_index_python #41
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Blast545 Great work!!
I left a bunch of comments, but most of them are stylistics or minor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some extra comments, but it's looking better!
What makes this PR harder to review than necessary is the mixture of unrelated changes. The patch does add tests as indicated by the title but also does some random code changes (arguably improvements) as well as refactorings to use different concepts (like I strongly suggest to keep changes separate and create different PRs for unrelated changes. That will make sure that each can be reviewed easily and can get merged quickly. |
Signed-off-by: Jorge J. Perez <jjperez@ekumenlabs.com>
I removed code change improvements for now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with Dirk's comment addressed and green CI.
Signed-off-by: Jorge J. Perez <jjperez@ekumenlabs.com>
Signed-off-by: Jorge J. Perez <jjperez@ekumenlabs.com>
Tests added to remaining functions in the package