Skip to content

Add empty host and trailing slash to windows file paths #71

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

Merged
merged 3 commits into from
Aug 17, 2022

Conversation

phil-davis
Copy link
Contributor

@phil-davis phil-davis commented Aug 11, 2022

This is a rebase of PR #25 with conflicts resolved...

In the 2nd commit, I have added some comments and simplified the windows_path regex in a similar way to what @peterpostmann has suggested in comment #25 (comment)

@phil-davis phil-davis self-assigned this Aug 11, 2022
@phil-davis phil-davis force-pushed the windows_host_handling branch from 5c36fbc to 52606c7 Compare August 11, 2022 05:23
@codecov
Copy link

codecov bot commented Aug 11, 2022

Codecov Report

Merging #71 (290c896) into master (5145a08) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master       #71   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines          139       143    +4     
=========================================
+ Hits           139       143    +4     
Impacted Files Coverage Δ
lib/functions.php 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@phil-davis phil-davis force-pushed the windows_host_handling branch from 52606c7 to 03e0c6c Compare August 11, 2022 05:27
@phil-davis phil-davis mentioned this pull request Aug 11, 2022
@phil-davis phil-davis marked this pull request as ready for review August 16, 2022 07:26
@phil-davis
Copy link
Contributor Author

@peterpostmann please have a look and comment if you agree or not with what I have done here.

@peterpostmann
Copy link
Contributor

I found one more issue: We are not catching the case, where it's just the drive letter, i.e. file:///C:
Do you want to make the whole part after the colon optional? /^(?<windows_path> [A-Z]:([\/\\\\].*)?)$/x

@phil-davis phil-davis force-pushed the windows_host_handling branch from 1089f0c to 9d8d47f Compare August 16, 2022 15:15
@phil-davis phil-davis requested a review from staabm August 16, 2022 15:16
@phil-davis
Copy link
Contributor Author

I found one more issue: We are not catching the case, where it's just the drive letter, i.e. file:///C: Do you want to make the whole part after the colon optional? /^(?<windows_path> [A-Z]:([\/\\\\].*)?)$/x

done in 3rd commit. The new test case fails before the change and passes with the change.

@phil-davis
Copy link
Contributor Author

IMO we are good-to-go here. @staabm reviewed the original PR previously. There has been commentary from people above that indicates they are OK with the direction this is going.

I will merge and release a patch version. That will let me move forward with PR #74 that will cause another release.

@phil-davis phil-davis merged commit 86e5e0e into sabre-io:master Aug 17, 2022
@phil-davis phil-davis deleted the windows_host_handling branch August 17, 2022 08:02
@phil-davis
Copy link
Contributor Author

Also see discussion in issue #81 PR #25 and issue #31

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