uri icon indicating copy to clipboard operation
uri copied to clipboard

Add empty host and trailing slash to windows file paths

Open phil-davis opened this issue 2 years ago • 1 comments

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

Now I will look at what we end up with in 2022!

phil-davis avatar Aug 11 '22 05:08 phil-davis

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%> (ø)

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

codecov[bot] avatar Aug 11 '22 05:08 codecov[bot]

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

phil-davis avatar Aug 16 '22 07:08 phil-davis

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

peterpostmann avatar Aug 16 '22 12:08 peterpostmann

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 avatar Aug 16 '22 15:08 phil-davis

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 avatar Aug 17 '22 08:08 phil-davis

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

phil-davis avatar Sep 19 '22 08:09 phil-davis