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

Split BagIt spec files to allow for CR, LF or CRLF line endings #37

Merged
merged 1 commit into from
Feb 23, 2022

Conversation

whikloj
Copy link
Owner

@whikloj whikloj commented Feb 22, 2022

Resolves #36

@codecov
Copy link

codecov bot commented Feb 22, 2022

Codecov Report

Merging #37 (26a7e1b) into main (683b5bd) will increase coverage by 3.37%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main      #37      +/-   ##
============================================
+ Coverage     83.66%   87.04%   +3.37%     
  Complexity      484      484              
============================================
  Files             7        7              
  Lines          1298     1297       -1     
============================================
+ Hits           1086     1129      +43     
+ Misses          212      168      -44     
Impacted Files Coverage Δ
src/AbstractManifest.php 86.66% <100.00%> (+5.78%) ⬆️
src/Bag.php 84.57% <100.00%> (+3.27%) ⬆️
src/BagUtils.php 93.00% <100.00%> (+1.16%) ⬆️
src/Fetch.php 91.77% <100.00%> (+4.32%) ⬆️

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 683b5bd...26a7e1b. Read the comment docs.

@whikloj
Copy link
Owner Author

whikloj commented Feb 23, 2022

@henning-gerhardt wanted to bring this one to your attention.

This library was not following the specification in that if you had made a file with only carriage returns at the end of lines it would not validate the bag. This changes the behaviour to allow for files with any of the allowed 3 endings.

However it changes the behaviour when reading bag-info files that have random new lines for formatting and not just wrapping. This will remove the existing newline character and replace it in the libraries' bag-info copy with the OS specific character (specified by PHP_EOL).

It will not affect the bag on disk (unless you update the bag, which is the same as before). However if you were wanting to know whether the lines ended with CR, LF or CRLF it would be impossible to tell now.

I don't think this would be an issue, but wanted to ping you as you had an interest in maintaining newline characters.

@henning-gerhardt
Copy link
Contributor

Thank you for mention me. I think this change is okay, as our usage scenario is only in creating new bags and in most cased not reading them. If we have some issues with this then I will create a new issue for this.

@whikloj whikloj merged commit 57eb89e into main Feb 23, 2022
@whikloj whikloj deleted the issue-36 branch February 23, 2022 20:49
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.

Bags with CR only line endings not handled correctly.
2 participants