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: properly handle file/url paths on Windows #645

Merged
merged 1 commit into from
Nov 9, 2023
Merged

fix: properly handle file/url paths on Windows #645

merged 1 commit into from
Nov 9, 2023

Conversation

G-Rath
Copy link
Collaborator

@G-Rath G-Rath commented Nov 6, 2023

Turns out that file -> url translation on Windows is busted, and that this is a hard problem that Go has an internal util for that has not yet been made public - I've done what apparently a number of other packages have done which is copying that helper into here and hoping one day it actually becomes public 😢

Note that until #646 is landed, there is no way to actually verify this is fixing the problem - #553 shows the result of both PRs being merged.

@G-Rath G-Rath mentioned this pull request Nov 6, 2023
@codecov-commenter
Copy link

codecov-commenter commented Nov 6, 2023

Codecov Report

Merging #645 (d5e7494) into main (0599ed3) will decrease coverage by 0.28%.
The diff coverage is 31.70%.

@@            Coverage Diff             @@
##             main     #645      +/-   ##
==========================================
- Coverage   78.42%   78.14%   -0.28%     
==========================================
  Files          80       81       +1     
  Lines        5826     5866      +40     
==========================================
+ Hits         4569     4584      +15     
- Misses       1058     1082      +24     
- Partials      199      200       +1     
Files Coverage Δ
internal/output/sarif.go 90.57% <100.00%> (+0.15%) ⬆️
internal/sourceanalysis/go.go 67.28% <100.00%> (+1.27%) ⬆️
internal/url/url.go 15.15% <15.15%> (ø)

... and 1 file with indirect coverage changes

Copy link
Collaborator

@another-rex another-rex left a comment

Choose a reason for hiding this comment

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

Lgtm!

@another-rex another-rex merged commit 161f26d into google:main Nov 9, 2023
8 checks passed
@G-Rath G-Rath deleted the fix-file-url-handling branch November 17, 2023 00:52
another-rex pushed a commit that referenced this pull request Nov 21, 2023
Unsurprisingly this has required a bunch of tests to be updated to
handle slightly different variations in file path handling - this
eventually resulted in me implementing an actual internal snapshot
testing package but I've not included that in here since its sizable on
its own; so please keep that in mind when reviewing . (see
https://github.com/G-Rath/osv-scanner/commit/1273da79e2e26a18d663da482dc5f09258e15c51
for a sneakpeek on what the snapshot-based testing looks like)

~Note that is failing because file -> url path translation is actually
busted; I've opened #645 to fix this and you can see the passing CI when
both of these changes are merged in #553~

Resolves #603
Resolves #553
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