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 filesystem::separator behavior #308

Merged
merged 5 commits into from
Mar 5, 2022
Merged

Fix filesystem::separator behavior #308

merged 5 commits into from
Mar 5, 2022

Conversation

mjcarroll
Copy link
Contributor

As it turns out, separator's behavior was not matching the ign-common5 behavior. Passing an empty string should return the preferred separator for the operating system. We were not currently testing for this.

@codecov
Copy link

codecov bot commented Feb 9, 2022

Codecov Report

Merging #308 (d364230) into main (ce7cb4a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #308   +/-   ##
=======================================
  Coverage   76.74%   76.75%           
=======================================
  Files          75       75           
  Lines       10325    10328    +3     
=======================================
+ Hits         7924     7927    +3     
  Misses       2401     2401           
Impacted Files Coverage Δ
src/Filesystem.cc 95.55% <100.00%> (+0.10%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce7cb4a...d364230. Read the comment docs.

@mjcarroll mjcarroll self-assigned this Feb 9, 2022
@mjcarroll mjcarroll force-pushed the remove_boost_tests branch 2 times, most recently from c7c2b44 to 798821b Compare February 9, 2022 16:02
@mjcarroll mjcarroll marked this pull request as ready for review February 9, 2022 19:29
src/Filesystem.cc Outdated Show resolved Hide resolved
src/Filesystem.cc Show resolved Hide resolved
Base automatically changed from remove_boost_tests to main February 10, 2022 13:10
@mjcarroll mjcarroll force-pushed the filesystem_bugfix branch 2 times, most recently from fb40333 to 0bc6478 Compare February 10, 2022 16:01
* Separator with an emptry string argument should return the preferred
separator for that platform
* Basename should return basename in the case with multiple trailing
slashes

Signed-off-by: Michael Carroll <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

I addressed review feedback in d453ab6. This looks good to me with happy CI. I'd like to get it in because it's currently not possible to download Fuel models that have directories without this PR.

@chapulina chapulina enabled auto-merge (squash) March 3, 2022 17:33
@chapulina chapulina added the 🌱 garden Ignition Garden label Mar 3, 2022
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
@chapulina chapulina merged commit b960986 into main Mar 5, 2022
@chapulina chapulina deleted the filesystem_bugfix branch March 5, 2022 00:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants