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

Improved Lift instances for Path/OsPath types #194

Closed
wants to merge 2 commits into from

Conversation

mmhat
Copy link
Contributor

@mmhat mmhat commented Sep 11, 2024

Extend the Lift instances such that they support parametric types and promoted data constructors for the two parameters of the Path/OsPath types.

The `Lift` instances support now parametic types and promoted data
constructors for the two parameters of the Path/OsPath types.
@NorfairKing
Copy link
Collaborator

The Path type is not meant to be used with anything other than Abs, Rel, File, Dir, see https://github.com/commercialhaskell/path?tab=readme-ov-file#the-data-types

(Why not use data kinds like data Type = File | Dir? Because that imposes an extension overhead of adding {-# LANGUAGE DataKinds #-} to every module you might want to write out a path type in. Given that one cannot construct paths of types other than these, via the operations in the module, it’s not a concern for me.)

Could you elaborate on your aims here?
As-is, I'm very hesitant to consider merging this.

@mmhat
Copy link
Contributor Author

mmhat commented Sep 22, 2024

@NorfairKing Well, I want to (re-)use the Path type with a custom base 🙂 I missed that part of the documentation though.

I have some application code that creates symlinks to filesystem entries in parent directories, so I construct a path containing ..s and stored that in a Path (WithParents b) Dir value. Recently I wanted to extract that code to an own library (I do not intend to release that on Hackage.) and when I wanted to add a quasi-quoter I got type errors since the Lift (Path b t) annotates the splice with the wrong type. Hence I opened this PR.

While I understand (and agree) that path does not aim to support use cases like that in general, would it be possible to only modify the Lift instance since it is the one thing one cannot work around? If the complexity added by this PR is deemed to much, would you be open to a change limiting the defined instances to:

instance Lift (Path Abs Dir)
instance Lift (Path Abs File)
instance Lift (Path Rel Dir)
instance Lift (Path Rel File)

@NorfairKing
Copy link
Collaborator

While I understand (and agree) that path does not aim to support use cases like that in general, would it be possible to only modify the Lift instance since it is the one thing one cannot work around? If the complexity added by this PR is deemed to much, would you be open to a change limiting the defined instances to:

I want to say yes but I find it difficult to estimate what kind of breakage that would cause.
Do you have any more insight into this?

@NorfairKing
Copy link
Collaborator

Closing this due to lack of activity. Feel free to reopen.

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