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

Fix parent_path() for empty paths and paths of length one #73

Merged
merged 8 commits into from
Jun 18, 2020

Conversation

jacobperron
Copy link
Member

Slightly unrelated, but this PR also adds tests for exists() to clarify it's expected behavior (36ebd99).

This fixes an issue discovered downstream: https://github.com/ros2/rosbag2/pull/345/files#r442416506

This clarifies the expected output for empty, current, and parent paths.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
In this case, return an empty path.
Previously, this would result in a segmentation fault.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Copy link
Contributor

@Karsten1987 Karsten1987 left a comment

Choose a reason for hiding this comment

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

lgtm with green CI

@jacobperron
Copy link
Member Author

Testing rcpputils:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron
Copy link
Member Author

Fixed expected output syntax on Windows: a2edff8

  • Windows Build Status

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron
Copy link
Member Author

Windows paths were trickier than I anticipated.
The changes in cf12223 handles absolute paths for the current drive (e.g. \foo or \) and for absolute paths with a letter drive specified (e.g. C:\foo or C:\)

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@Karsten1987 @wjwwood Can you take another look? Thanks!

@jacobperron jacobperron merged commit 8c99e75 into master Jun 18, 2020
@jacobperron jacobperron deleted the jacob/parent_path_exists branch June 18, 2020 22:28
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.

3 participants