haskell / alex

A lexical analyser generator for Haskell
https://hackage.haskell.org/package/alex
BSD 3-Clause "New" or "Revised" License
298 stars 82 forks source link

Add a stock Ord instance for AlexPosn #234

Closed Kleidukos closed 1 year ago

Kleidukos commented 1 year ago

Close #233

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

andreasabel commented 1 year ago

@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 commented 1 year ago

https://hackage-search.serokell.io/?q=instance+Ord+AlexPosn gives the following conflicts:

Screenshot 2023-05-14 at 19 18 45
andreasabel commented 1 year ago

Resolved the conflict; planning to release this as 3.3.0.

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

Kleidukos commented 1 year ago

@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)
[…]
andreasabel commented 1 year ago

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 commented 1 year ago

ooooooh I see! Thank you :)

andreasabel commented 1 year ago

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

Kleidukos commented 1 year ago

@andreasabel understood, I'll merge it then.

andreasabel commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

Ericson2314 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

Release candidate for 3.3.0.0 at: