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: file paths on windows conflict with the ast escape rune #12

Merged
merged 1 commit into from
Dec 8, 2020

Conversation

djgilcrease
Copy link
Contributor

@djgilcrease djgilcrease commented Dec 8, 2020

fix: file paths on windows conflict with the ast escape rune so we need to keep paths in the linux style

Related-To: goreleaser/goreleaser#1935

PS C:\Users\digit\projects\fileglob> go test -v ./...
=== RUN   TestGlob
=== RUN   TestGlob/real
=== RUN   TestGlob/simple
=== RUN   TestGlob/single_file
=== RUN   TestGlob/super_asterisk
=== RUN   TestGlob/alternative_matchers
=== RUN   TestGlob/character_list_and_range_matchers
=== RUN   TestGlob/nested_matchers
=== RUN   TestGlob/single_symbol_wildcards
=== RUN   TestGlob/direct_match
=== RUN   TestGlob/direct_match_wildcard
=== RUN   TestGlob/direct_no_match
=== RUN   TestGlob/escaped_direct_no_match
=== RUN   TestGlob/direct_no_match_escaped_wildcards
=== RUN   TestGlob/no_matches
=== RUN   TestGlob/empty_folder
=== RUN   TestGlob/escaped_asterisk
=== RUN   TestGlob/escaped_curly_braces
=== RUN   TestGlob/invalid_pattern
=== RUN   TestGlob/prefix_is_a_file
=== RUN   TestGlob/match_files_in_directories
=== RUN   TestGlob/match_directories_directly
=== RUN   TestGlob/match_empty_directory
=== RUN   TestGlob/pattern_ending_with_star_and_subdir
--- PASS: TestGlob (0.01s)
    --- PASS: TestGlob/real (0.00s)
    --- PASS: TestGlob/simple (0.00s)
    --- PASS: TestGlob/single_file (0.00s)
    --- PASS: TestGlob/super_asterisk (0.00s)
    --- PASS: TestGlob/alternative_matchers (0.00s)
    --- PASS: TestGlob/character_list_and_range_matchers (0.00s)
    --- PASS: TestGlob/nested_matchers (0.00s)
    --- PASS: TestGlob/single_symbol_wildcards (0.00s)
    --- PASS: TestGlob/direct_match (0.00s)
    --- PASS: TestGlob/direct_match_wildcard (0.00s)
    --- PASS: TestGlob/direct_no_match (0.00s)
    --- PASS: TestGlob/escaped_direct_no_match (0.00s)
    --- PASS: TestGlob/direct_no_match_escaped_wildcards (0.00s)
    --- PASS: TestGlob/no_matches (0.00s)
    --- PASS: TestGlob/empty_folder (0.00s)
    --- PASS: TestGlob/escaped_asterisk (0.00s)
    --- PASS: TestGlob/escaped_curly_braces (0.00s)
    --- PASS: TestGlob/invalid_pattern (0.00s)
    --- PASS: TestGlob/prefix_is_a_file (0.00s)
    --- PASS: TestGlob/match_files_in_directories (0.00s)
    --- PASS: TestGlob/match_directories_directly (0.00s)
    --- PASS: TestGlob/match_empty_directory (0.00s)
    --- PASS: TestGlob/pattern_ending_with_star_and_subdir (0.00s)
=== RUN   TestQuoteMeta
--- PASS: TestQuoteMeta (0.00s)
=== RUN   TestStaticPrefix
--- PASS: TestStaticPrefix (0.00s)
=== RUN   TestContainsMatchers
--- PASS: TestContainsMatchers (0.00s)
=== RUN   TestValidPattern
--- PASS: TestValidPattern (0.00s)
PASS
ok      github.com/goreleaser/fileglob  0.043s

@codecov
Copy link

codecov bot commented Dec 8, 2020

Codecov Report

Merging #12 (f6ee601) into main (b27e0ce) will increase coverage by 0.69%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #12      +/-   ##
==========================================
+ Coverage   82.97%   83.67%   +0.69%     
==========================================
  Files           2        2              
  Lines          94       98       +4     
==========================================
+ Hits           78       82       +4     
  Misses          9        9              
  Partials        7        7              
Impacted Files Coverage Δ
glob.go 83.33% <100.00%> (+0.87%) ⬆️
prefix.go 84.21% <100.00%> (+0.42%) ⬆️

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 b27e0ce...f6ee601. Read the comment docs.

glob.go Outdated Show resolved Hide resolved
@caarlos0
Copy link
Member

caarlos0 commented Dec 8, 2020

LGTM, @erikgeiser can you give a look as well?

Thanks for the PR @djgilcrease 🙏

@erikgeiser
Copy link
Member

Do I understand correctly that the idea behind this is to always treat paths like Unix paths because Windows can also handle them?

@djgilcrease
Copy link
Contributor Author

djgilcrease commented Dec 8, 2020

Do I understand correctly that the idea behind this is to always treat paths like Unix paths because Windows can also handle them?

Yes, and because The glob ast from github.com/gobwas/glob only works properly with linux paths

@erikgeiser
Copy link
Member

Is this a bug in https://github.com/gobwas/glob? We already pass filepath.Seperator to the Compile function.

@djgilcrease
Copy link
Contributor Author

Is this a bug in https://github.com/gobwas/glob? We already pass filepath.Seperator to the Compile function.

kind of, because filepath.Seperator on windows == \ that causes it to conflict with the escape rune https://github.com/gobwas/glob/blob/master/syntax/lexer/lexer.go#L14 and I do not see any easy way to fix this in gobwas/glob so I decided to fix it here.

Copy link
Member

@erikgeiser erikgeiser left a comment

Choose a reason for hiding this comment

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

lgtm

glob.go Outdated Show resolved Hide resolved
@djgilcrease djgilcrease force-pushed the fix-windows-path branch 2 times, most recently from 1e9561a to fda6d94 Compare December 8, 2020 19:58
@caarlos0
Copy link
Member

caarlos0 commented Dec 8, 2020

I would also say before merging this we should setup CI to test on windows as well... not sure if supported by github actions though

@caarlos0 caarlos0 mentioned this pull request Dec 8, 2020
@erikgeiser
Copy link
Member

Sure it is but we don't actually have that many tests that use the actual file system.

@djgilcrease
Copy link
Contributor Author

I would also say before merging this we should setup CI to test on windows as well... not sure if supported by github actions though

I would hope it is supported since MS owns github =P

I added a windows style path test as I ran into issues with this when I was fixing nfpm to be able to run on windows. This project does not actually read the file system, but nfpm & goreleaser should have tests running in windows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants