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 #36; correctness wrt symlinks #38

Merged
merged 2 commits into from
Jan 20, 2020

Conversation

timotheecour
Copy link
Collaborator

@timotheecour timotheecour commented Jan 17, 2020

@haltcase
Copy link
Owner

haltcase commented Jan 18, 2020

@timotheecour thanks for the fix. Looks like this affects the ability to find paths case-insensitively on macOS (and likely Windows¹), can you review that?

https://travis-ci.com/citycide/glob/jobs/277020797#L208

¹ seems AppVeyor has made VS projects the default mode within the last year, which broke the Windows tests. I should look into putting everything into GitHub actions at some point.

@timotheecour
Copy link
Collaborator Author

timotheecour commented Jan 18, 2020

@citycide PTAL
it's not 100% ideal but should be correct for all purposes and definitely better than behavior prior to this PR; ideally we'd have a way to resolve case (but not resolve symlinks) but no idea how to do that; it should be correct though; eg:

on OSX:
mkdir TEMP
ls temp # works

I've asked on stackverflow here https://stackoverflow.com/questions/59797065/how-to-get-original-case-of-a-path-on-osx-eg-tmp-tmp how we can resolve a path to its original case (eg: mkdir TEMP; getOrigCase(temp) => TEMP) ; we can improve later if there's a way

¹ seems AppVeyor has made VS projects the default mode within the last year, which broke the Windows tests. I should look into putting everything into GitHub actions at some point.

how about switching to azure pipelines, like Nim did? (so you only need 1 pipeline instead of 2, as it's cross platform)

@haltcase
Copy link
Owner

haltcase commented Jan 18, 2020

@timotheecour hmm.. I suppose it's alright not to maintain the casing if it's not reasonable to do it without also resolving symlinks. I'm not even really sure how important it is tbh, I just always considered it sensible. Maybe we wait a bit and see if the universe gives us an answer via your SO question.

As for the CI provider, travis & github actions both also support macOS, windows & linux at this point. I don't know much about azure pipelines yet, but the easiest option (to combine providers) would be to just enable windows in travis.

@timotheecour
Copy link
Collaborator Author

ping now that travis supports (osx, windows, linux); resolving symlinks as was the case before this PR was bad (eg caused #36)
not maintaining the case on OSX is ok since OSX is case insensitive anyway:
mkdir FOO && cd foo works; it's just that the original case is not maintained ; if the stackoverflow question gets an answer we can always improve this here

@haltcase haltcase merged commit 068369b into haltcase:master Jan 20, 2020
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.

[critical] walkGlob doesn't work with symlinks in pattern; unlike ls, os.walkDir etc
2 participants