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

Merged
merged 35 commits into from
Jan 19, 2025

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.
  • Most command environments now have a rootPath :: RootPath and use that path as root for all operations, no longer relying on CWD for this purpose.
    • Some commands, such as registry, don't need a root path.
  • 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)

This was referenced Nov 1, 2024
@fsoikin
Copy link
Collaborator Author

fsoikin commented Jan 19, 2025

Alright, I finally figured out the Windows failure, and it turned out to be an actual potential problem, not just a silly forward vs. back slash issue. I added special handling for it and some tests.

@fsoikin fsoikin requested a review from f-f January 19, 2025 05:29
Copy link
Member

@f-f f-f left a comment

Choose a reason for hiding this comment

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

Looks great now - awesome job 👏

@f-f f-f merged commit c723832 into purescript:master Jan 19, 2025
5 checks passed
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.

2 participants