fix(stats): handle windows file project roots#453
Conversation
jupblb
left a comment
There was a problem hiding this comment.
Thank you very much for this contribution. I've left some comments. But, to be honest, even if these issues are fixed I'm a bit against merging this PR just yet.
We should first:
- Figure out if someone is willing to maintain support for Windows long-term (if that's you @jojosenthusiast please let us know)
- Add Windows host to our CI (doesn't have to be the full suite, like the one we're using with Nix, just the essential checks)
If there is interest in having support for Windows then I'm cool with adding it, but the existing maintainers currently do not have the capacity needed to ensure this keeps working.
| // For all other inputs it preserves the previous behavior of using | ||
| // url.Parse(rawProjectRoot).Path. | ||
| func ProjectRootToLocalPath(rawProjectRoot string) (string, error) { | ||
| if isWindowsFileURIWithDriveLetter(rawProjectRoot) { |
There was a problem hiding this comment.
Can we invert this flow and try url.Parse(rawProjectRoot) first, then fall back only if parsing fails with the known Windows drive-root shape?
Right now the special-case runs before url.Parse, so it catches inputs that previously parsed successfully. For example, file://x:123/share previously produced path /share, but this helper would now return x:123/share. That conflicts with the comment below saying all other inputs preserve url.Parse(rawProjectRoot).Path.
| // ponytail: only the "file://X:..." form is handled here; the | ||
| // canonical "file:///X:/..." form parses fine via url.Parse and is | ||
| // out of scope for this fix. | ||
| return rawProjectRoot[len("file://"):], nil |
There was a problem hiding this comment.
This should probably run through url.PathUnescape before returning. The normal url.Parse(...).Path branch gives callers the decoded path, but this branch returns the raw URI suffix. A root like file://D:%5Cdev%5Cmy%20project would currently become D:%5Cdev%5Cmy%20project instead of D:\dev\my project.
| // isWindowsFileURIWithDriveLetter reports whether s matches "file://X:..." | ||
| // where X is an ASCII letter and the position after the scheme is NOT a third | ||
| // slash (which would indicate the canonical "file:///..." form). | ||
| func isWindowsFileURIWithDriveLetter(s string) bool { |
There was a problem hiding this comment.
This predicate seems too broad for the stated fix. It accepts any file://<letter>: prefix, including drive-relative paths like file://D:relative, which previously failed parsing and are not absolute project roots.
Could we require the character after the drive colon to be a path separator (\ or /), or otherwise only enter this fallback after url.Parse has failed? That keeps the workaround scoped to the issue shape.
Fixes #282
Reproduction
scip statscan fail when a SCIP index contains a Windows project root likefile://D:\dev\project, becauseurl.Parsetreats the drive-letter colon as an invalid host port.Fix summary
file://D:\...form before falling back tourl.Parse(...).Pathfor normal file URIs.scip statsline-counting and snapshot formatting paths.countLinesOfCodepath where an existing parsed project root leftlocalSourceempty beforeos.Stat("").cmd/scipseam tests coveringcountLinesOfCodebehavior.Tests run
git diff --check? passgofmt -lon changed Go files ? pass, no outputgo test ./bindings/go/scip -run TestProjectRootToLocalPath -count=1? passgo test ./cmd/scip -run TestCountLinesOfCode -count=1? passgo test ./bindings/go/scip ./bindings/go/scip/testutil ./cmd/scip? fails only atcmd/scip.TestCLIReferenceInSync, unrelated to this diff because CLI help/docs files are not changedRisk
Low. The special case is limited to the reported non-canonical Windows
file://<drive>:form; other file URIs continue through Go's URL parser.Non-goals
file:///D:/...behavior beyond preserving existing parser behavior.