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

Add a stock Ord instance for AlexPosn #234

Merged
merged 6 commits into from
May 24, 2023

Conversation

Kleidukos
Copy link
Member

@Kleidukos Kleidukos commented May 1, 2023

Close #233

This PR adds an Ord instance to the generated and the internal AlexPosn struct.

@Kleidukos Kleidukos requested a review from andreasabel May 1, 2023 19:31
@andreasabel andreasabel added this to the 3.2.8 milestone May 1, 2023
@andreasabel andreasabel requested a review from Ericson2314 May 2, 2023 06:53
@andreasabel andreasabel added the wrapper Concerning wrapper code label May 2, 2023
@andreasabel
Copy link
Member

@Ericson2314 :
This change will break compilation of project that add an orphan instance Ord AlexPosn, so I want a second opinion, e.g. what versioning concerns (3.2.8 vs. 3.3).

@andreasabel andreasabel self-assigned this May 14, 2023
@andreasabel andreasabel modified the milestones: 3.2.8, 3.3.0 May 14, 2023
@andreasabel andreasabel force-pushed the 233-ord-instance-for-AlexPosn branch from a143563 to 9ec6008 Compare May 14, 2023 17:25
@andreasabel
Copy link
Member

andreasabel commented May 14, 2023

Resolved the conflict; planning to release this as 3.3.0.

Candidate at: https://hackage.haskell.org/package/alex-3.3.0/candidate

Copy link
Member

@andreasabel andreasabel left a comment

Choose a reason for hiding this comment

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

Please clarify what this PR wants to achieve and argue why it achieves it.
Also a test please.

src/ParseMonad.hs Show resolved Hide resolved
@Kleidukos
Copy link
Member Author

@andreasabel I'm slightly lost wrt the test suite. Despite having changed the wrapper, I don't think the test suite actually failed on the content change of the generated lexer, despite said content having changed in my tree.

❯ grep -R "deriving (Eq, Show, Ord)"
basic_typeclass.n.hs:        deriving (Eq, Show, Ord)
basic_typeclass.g.hs:        deriving (Eq, Show, Ord)
[…]

Copy link
Member

@andreasabel andreasabel left a comment

Choose a reason for hiding this comment

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

Thanks!
I added a test for the availability of the instance Ord AlexPosn.

@andreasabel
Copy link
Member

I don't think the test suite actually failed on the content change of the generated lexer,

Maybe you expected a "golden value" test, which does not exist here. The testsuite just generates lexers from .x files, I think it does not even try to run them. The generated lexers are not checked in as golden values.

@Kleidukos
Copy link
Member Author

ooooooh I see! Thank you :)

@andreasabel
Copy link
Member

Can't get hold of John @Ericson2314 it seems. Maybe then we just go ahead with the 3.3.0 plan.

@Kleidukos
Copy link
Member Author

@andreasabel understood, I'll merge it then.

@Kleidukos Kleidukos merged commit 9af9d09 into haskell:master May 24, 2023
@Kleidukos Kleidukos deleted the 233-ord-instance-for-AlexPosn branch May 24, 2023 13:55
@andreasabel
Copy link
Member

@andreasabel understood, I'll merge it then.

Maybe the final "hit the button" should be priviledge of the maintainers (stern look).
Or do you want to apply for co-maintainership?

@Kleidukos
Copy link
Member Author

My apologies, I was operating under a "take responsibility of your contributions" model.

I am a very poor user of Alex right now, but perhaps after a few other contributions I would certainly consider helping out with maintenance. :)

@andreasabel
Copy link
Member

I would have reorganized the commits a bit so that testcase is in the same commit with the change, but so be it now.

perhaps after a few other contributions I would certainly consider helping out with maintenance. :)

Contributions are very welcome!
I think it is also nice if maintainers have sparring partners, so that one can get a second option.

@Ericson2314
Copy link
Collaborator

@andreasabel I am very sorry I did not respond to this in time.

@Ericson2314
Copy link
Collaborator

Looking at it after the fact, the change and the version number bump seem good to me. The state of the version numbers on Alex and Happy is a little weird, but hopefully we can consistently get back to 4 digit ones so the interpretation of the PVP is most obvious.

@andreasabel
Copy link
Member

@Ericson2314

@andreasabel I am very sorry I did not respond to this in time.

No worries!

Looking at it after the fact, the change and the version number bump seem good to me. The state of the version numbers on Alex and Happy is a little weird, but hopefully we can consistently get back to 4 digit ones so the interpretation of the PVP is most obvious.

We can release this as 3.3, 3.3.0, or 3.3.0.0. Technically, this is a strictly increasing version sequence, but non-experts (like my former self) would consider these equal.

Historically, Alex had both x.y (2.2, 2.3, 3.0) and x.y.0 (2.1.0, 3.1.0, 3.2.0) with x.y.0 dominant recently, so I went for 3.3.0.
However, we could continue our 4-digit streak and switch to exactly 4 digits always, so 3.3.0.0 would be next.

I take your statement as a vote for 3.3.0.0, @Ericson2314, is this your intention?

@andreasabel
Copy link
Member

Release candidate for 3.3.0.0 at:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wrapper Concerning wrapper code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deriving Ord for AlexPosn
3 participants