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

make osp :: QuasiQuoter valid as a pattern #210

Merged
merged 1 commit into from
Dec 2, 2023
Merged

make osp :: QuasiQuoter valid as a pattern #210

merged 1 commit into from
Dec 2, 2023

Conversation

lawbel
Copy link
Contributor

@lawbel lawbel commented Nov 29, 2023

Resolves issue #205

Notes:

  • Alternative to the quote [p|...|] is something like

    viewP (appE (varE '(==)) (lift osp')) (conP 'True [])

    Although this may then need CPP to adjust for conP gaining an extra argument in template-haskell v2.18.0.0. Not sure which GHC version this changed in, but it's due to the ability to bind a type parameter with a constructor like

    foo :: Maybe a -> String
    foo (Just @a x) = "just"

    Given that, I thought the quote would be clearer. I'm assuming it should be easier to maintain as well, as it should avoid the need to update with any TH changes?

  • TemplateHaskellQuotes and ViewPatterns extensions both go back to GHC v6.x which should be plenty for back-compat?

  • Any other changes that should be made? Add some suitable tests?

  • Should we change the other QuasiQuoters as well? Make similar changes to osstr and pstr?

@Bodigrim
Copy link
Contributor

I think filepath should define just

osp :: QuasiQuoter
osp = OS.osstr

pstr :: QuasiQuoter
pstr = OS.osstr

while the actual quasiquoter to be improved in https://github.com/haskell/os-string.

@hasufell
Copy link
Member

I think filepath should define just

osp :: QuasiQuoter
osp = OS.osstr

pstr :: QuasiQuoter
pstr = OS.osstr

while the actual quasiquoter to be improved in https://github.com/haskell/os-string.

We run isValid at compile time here, which we don't in the osstr quasiquoters.

@hasufell
Copy link
Member

hasufell commented Dec 1, 2023

@lawbel we probably want this patch for os-string as well

@hasufell hasufell merged commit 0682302 into haskell:master Dec 2, 2023
17 of 20 checks passed
hasufell added a commit to haskell/os-string that referenced this pull request Dec 2, 2023
@lawbel
Copy link
Contributor Author

lawbel commented Dec 4, 2023

@hasufell sorry I wasn't able to look at this over the weekend. I see you've done a PR for os-string as well, so I guess this is all done then? I appreciate the help in getting this over the line 😄

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.

3 participants