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: don't rely on string context for package source #170

Merged
merged 2 commits into from
Jun 15, 2023

Conversation

dixslyf
Copy link
Contributor

@dixslyf dixslyf commented Jun 15, 2023

Fixes #169.

Summary:

  • String contexts do not seem like a good fit for the package source type.
  • Removes haskell-source-type and uses with types; either path str for package source type.
  • Apart from haskell-source-type's check, isPathUnderNixStore was used to differentiate between a path to a package source and a version string. All usage of isPathUnderNixStore has been replaced with lib.types.path.check.

Todo:

  • Add test for when projectRoot is set (I haven't looked into how testing is done in haskell-flake; feel free to add one).
  • Does types.path cover all valid paths? If not, we need to add tests.

Before this commit, the type of a package source (haskell-source-type)
depended on string contexts in its `check` function. However, string
contexts do not work well to represent a path to a package source. For
instance, "hello ./subdirectory" would have a string context, but is
obviously not a valid path.

Furthermore, a `nix` bug (NixOS/nix#8428)
related to string contexts triggers an issue where setting the project
root causes a type error with haskell-source-type (srid#169).

This commit replaces haskell-source-type with `with type; either path str`
and instances of `isPathUnderNixStore` with `lib.types.path.check` to
avoid using string contexts.
@srid
Copy link
Owner

srid commented Jun 15, 2023

#169 (comment)

I was using that before. It didn't work for some reason I can't recall right now. IIRC types.path doesn't cover all valid paths, maybe that's it.

I observed this in https://github.com/srid/emanote. Using your PR in emanote, I see no regressions. So think this PR is good to merge.

@srid srid merged commit c5e908e into srid:master Jun 15, 2023
3 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.

Setting projectRoot causes a type error
2 participants