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

Ensure we start working with an absolute path #27

Merged
merged 5 commits into from
Jul 7, 2021
Merged

Ensure we start working with an absolute path #27

merged 5 commits into from
Jul 7, 2021

Conversation

whikloj
Copy link
Owner

@whikloj whikloj commented Jul 5, 2021

Resolves #26

@codecov
Copy link

codecov bot commented Jul 6, 2021

Codecov Report

Merging #27 (b480cbd) into main (94b6068) will increase coverage by 1.25%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main      #27      +/-   ##
============================================
+ Coverage     82.47%   83.72%   +1.25%     
- Complexity      481      484       +3     
============================================
  Files             7        7              
  Lines          1295     1303       +8     
============================================
+ Hits           1068     1091      +23     
+ Misses          227      212      -15     
Impacted Files Coverage Δ
src/Bag.php 81.06% <100.00%> (+1.97%) ⬆️
src/BagUtils.php 91.11% <100.00%> (+0.74%) ⬆️

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 94b6068...b480cbd. Read the comment docs.

@whikloj
Copy link
Owner Author

whikloj commented Jul 6, 2021

@asmecher do you want to give this a try? I apologize that this simple change has come at the cost of removing PHP 7.2 support. I'll cut this as a ^3.0 release

@whikloj
Copy link
Owner Author

whikloj commented Jul 6, 2021

@asmecher I updated main to remove PHP 7.2 support and fix all the other underlying bits. So now this PR is just about the issue at hand. Please have a look and let me know if I am meeting the use-case.

@asmecher
Copy link

asmecher commented Jul 7, 2021

Thanks, @whikloj! I tested this by applying https://github.com/whikloj/BagItTools/pull/27.diff to my Composer-based installation (version 2.1.0). The patch applied successfully (with a bit of noise due to upstream changes):

patching file src/Bag.php
Hunk #3 succeeded at 523 with fuzz 2.
Hunk #4 succeeded at 1176 (offset 2 lines).
patching file src/BagUtils.php
Hunk #2 succeeded at 125 with fuzz 2.
Hunk #3 succeeded at 140 (offset 1 line).
patching file tests/BagInternalTest.php
patching file tests/BagTest.php
Hunk #1 succeeded at 18 with fuzz 1 (offset -1 lines).
Hunk #2 succeeded at 30 (offset -1 lines).
Hunk #3 succeeded at 94 (offset -1 lines).
Hunk #4 succeeded at 774 (offset -11 lines).
Hunk #5 succeeded at 968 (offset -19 lines).
patching file tests/BagUtilsTest.php
Hunk #1 succeeded at 75 (offset -1 lines).

I tested this with a relative path and it worked fine where it previously failed.

Recent releases of our software require PHP7.3, so the PHP7.2 loss isn't a problem.

Thanks for the quick turn-around on this, I appreciate it!

@whikloj whikloj merged commit ca234e6 into main Jul 7, 2021
@whikloj
Copy link
Owner Author

whikloj commented Jul 7, 2021

I'll tag a 3.0.1 in the morning

@whikloj whikloj deleted the issue-26 branch July 7, 2021 01:29
@asmecher
Copy link

asmecher commented Jul 8, 2021

Thanks, @whikloj, much appreciated!

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.

Creation of bag with relative bag fails unless ./ prefix is used
2 participants