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

Stricter path types for stricter path-related logic #1296

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

fsoikin
Copy link
Collaborator

@fsoikin fsoikin commented Oct 25, 2024

Description of the change

Summary of prior discussion:

  • Paths in Spago are handled in a bit of an ad-hoc way. To name a few examples: some places use local/partial paths and rely on current directory being the right one; other places construct paths out of pieces; the cwd value is unsafePerformEffected at init and then ends up baked into various values around the code.
  • This would be a problem for implementing spago script.
  • This would be a problem for implementing In monorepos, allow running commands from package directories #1237
  • We agreed to introduce stricter types to represent paths in Spago code.

In this pull request:

  • New module Spago.Path
  • Three new types for logically different kinds of paths - RootPath, LocalPath, and GlobalPath. See comments on the types for explanation.
  • All lower-level utilities, such as Spago.FS, take strongly typed paths as parameters.
  • This creates ripple effects, forcing the whole codebase to use strongly typed paths.
  • Every command environment now has a rootPath :: RootPath and uses that path as root for all operations, no longer relying on CWD for this purpose.
  • Halogen apps are not using the new types. I'm not yet familiar enough with them to make the changes.

Checklist:

  • Added the change to the "Unreleased" section of the changelog
  • [ ] Added some example of the new feature to the README
  • Added a test for the contribution (if applicable)


try (SyncFS.readTextFile UTF8 entry.path) >>= traverse_ \gitignore -> do
try (FS.readTextFileSync entry.path) >>= traverse_ \gitignore -> do
let
gitignored = testGlob <$> (splitGlob $ gitignoreFileToGlob base gitignore)
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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.

, package :: Core.PackageConfig
, doc :: YamlDoc Core.Config
, doc :: Maybe (YamlDoc Core.Config)
Copy link
Collaborator Author

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.

This was referenced Nov 1, 2024
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.

1 participant