-
Notifications
You must be signed in to change notification settings - Fork 132
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
Stricter path types for stricter path-related logic #1296
base: master
Are you sure you want to change the base?
Conversation
|
||
try (SyncFS.readTextFile UTF8 entry.path) >>= traverse_ \gitignore -> do | ||
try (FS.readTextFileSync entry.path) >>= traverse_ \gitignore -> do | ||
let | ||
gitignored = testGlob <$> (splitGlob $ gitignoreFileToGlob base gitignore) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found this slightly concerning place here. We're checking globs against other globs in an effort to verify if this gitignore conflicts with any of the patterns in parameters, and this doesn't always work as you would expect it to.
> mm = require('micromatch')
> mm.matcher("**/foo/bar")("**/foo/bar")
true
> mm.matcher("**/foo/bar")("**/foo/*")
false
Obviously "**/foo/*"
overlaps "**/foo/bar"
, but the matcher can't see it, because it expects the second parameter to be a file name, not a pattern.
@@ -111,3 +111,50 @@ Learn by doing and get your hands dirty! | |||
[f-f]: https://github.com/f-f | |||
[discord]: https://purescript.org/chat | |||
[spago-issues]: https://github.com/purescript/spago/issues | |||
|
|||
## Working with file paths |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like this documentation to exist, but I'm not sure this is the right place for it and I couldn't find a better one.
…l fear no forward slashes
, package :: Core.PackageConfig | ||
, doc :: YamlDoc Core.Config | ||
, doc :: Maybe (YamlDoc Core.Config) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is not actually part of the path types work, but is accidentally left over from my work on spago script
, where I'd like to have synthetic workspaces without a YAML doc on the file system, so I made it optional. Use sites that actually need it will die
if it's absent, explaining that it's an internal error (see configDocMissingErrorMessage
in this file).
I would like to keep it here if you don't mind, since it's a small change and already made. If you insist, I can, of course, take it out and add back in when I get back to spago script
.
Description of the change
Summary of prior discussion:
cwd
value isunsafePerformEffect
ed at init and then ends up baked into various values around the code.spago script
.In this pull request:
Spago.Path
RootPath
,LocalPath
, andGlobalPath
. See comments on the types for explanation.Spago.FS
, take strongly typed paths as parameters.rootPath :: RootPath
and uses that path as root for all operations, no longer relying on CWD for this purpose.Checklist:
[ ] Added some example of the new feature to theREADME