-
Notifications
You must be signed in to change notification settings - Fork 56
Bump bounds to build with GHC 9.8, and fix base64 support #98
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
Conversation
|
Hi @qnikst, any chance you could take a look at this? It would be great to get a GHC 9.8 compatible version on Hackage. |
Changes taken from qnikst#98 (but we don't want to rely on foreign forks.)
intricate
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
Thanks for PR! I agree with comments of @intricate. <0.5 was for purpose, the same is <1.1 in the current. For the support we should either drop all CPP checks of base64 in the codebase or extend it to keep support of the wider range. In the current case I don’t see a huge need in the wider range. for splitting PR I’m ok with updating stack and base64 in one PR, as commits are separate and they are logically related. once that will be done I’ll be happy to merge, fix CI and release a new version. |
|
Cool!
Now that I look at this again, I'm not sure why I removed some of the CPP. I think it might be slightly incorrect on I think this is worth doing, since current Stackage release generations still use Does that sound okay @qnikst ? Let me know and I'll add (correct) CPP to support all versions. |
|
Sounds great, thanks @thomasjm! |
8f87a8e to
f91e223
Compare
|
Okay @qnikst, this is ready for another look. I figured out why I removed the CPP, it is not necessary for the encode direction since the API didn't change; it remains needed for the decode direction. I did a couple more things which you can see commit by commit:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me.
edit: Nevermind my comment about non-exhaustive pattern matches.
| (AUTH _ _ _) -> error "BUG: AUTH should not get matched here" | ||
| (RETR _) -> error "BUG: RETR should not get matched here" | ||
| (TOP _ _) -> error "BUG: TOP should not get matched here" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about my previous comment on these non-exhaustive pattern matches.
I didn't notice that some of these patterns are being matched at the sendCommand function-definition "level" and then redundantly being matched in this case again.
I wonder if this should be further addressed in a follow up. To make this code more clear, it seems like there should only be one definition of sendCommand and a single case pattern match on command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recent GHC versions understand these matches are redundant and warn about them, so I was just fixing a couple warnings. Further refactoring could certainly happen as a follow-up.
|
Hi @qnikst, this should be ready to go! |
|
Great job, thanks! |
Bumps a few bounds to build with GHC 9.8 on the latest Stackage resolver (nightly-2024-07-22).
Also fixes the support for
base64 > 0.4. The initial support was landed in #91, but seems not to match what eventually happened in thebase64package. Version0.5never existed, as the version was bumped all the way to1.0with a breaking API change.Also checks in a small
stack.yamlfile to make it easy to test with the latest resolver.