persistent-template: Use the stock strategy when deriving Show/Read f… #1106
Conversation
|
Cool, do you have thoughts on what the version number should be? |
|
well, it's a breaking change, but since it fixes a bug that is also a breaking change that came in a non-breaking manner, I think we're fine to release a patch version and deprecate the versions that contained the breaking behavior. |
|
Sounds good |
|
Hi, as someone whose software has been broken by this, and not because we were relying on the output of the show instance in our code that I'm aware of, but due to something in persistent itself breaking, requiring manual database hacking (I will double check later today but a LTS update from 14.27->16.11 did break our data), this should be a minor release number change, so people who assumed it was an intentional change and migrated their data accordingly already can see it more easily since a new minor release will likely break compilation on the next LTS update. Here's what happened to https://github.com/Carnap/Carnap, specifically Carnap-Server:
Update: I may have been mistaken, and we might have been keying on the output of |
|
Yes, that's right from @lf-: our issue is that one of the constructors for a datatype that we're using There does still seem to be a potential worry that |
|
Ah ok, I think this is a strong case to do a breaking version bump for this |
|
@MaxGabriel I'm confused by what you mean - it seems to me that @lf- and @gleachkr so far is saying a minor version bump is appropriate, which is my intuition as well. |
|
Ok I'm kinda confused, this is how I was reading people:
I am taking that to mean, in the PVP versioning schema (A.B.C.D/MAJOR.MAJOR.MINOR.PATCH), you were saying to just bump PATCH.
This part confuses me. LTS Haskell automatically picks up minor version bumps (https://github.com/commercialhaskell/lts-haskell#how-it-works), but it sounds like what @lf- wanted was for this new version to not be picked up until the next LTS version (where people would assume breaking changes). But for that to happen, it needs a major (either A or B in A.B.C.D) |
|
@MaxGabriel: What I meant is that I wanted it to cause version bounds to break, encouraging reading the changelog, but I was not aware of the implications for LTS Haskell users as I primarily work with Nix. I believe the fix should require a major upgrade, even though it would delay LTS availability since, for example, if an app already on the buggy version newly introduces one of the affected use cases, thereby becoming affected by this bug, it would only have data needing migration following the fix release. Regardless of our choice, migrations/fixes will need to happen for all apps using show that have used the affected version, so our role is to most effectively notify people that these migrations are needed. |
|
Ok cool, so I think I’m in favor of a major (B in A.B.C.D to be specific) bump. That said, I am more open to making lots of major version bumps than most people. Now that it’s clear there is a disagreement, can the people who prefer C or D version bumps weigh in? I am inclined to agree with @lf- |
|
I don't have a strong preference. We can put this in with 2.11, or we can get it out-the-door quicker with a 2.10 minor release. |
|
Ok cool, let's just do 2.11. I'll update the PR accordingly |
|
Ok, updated to 2.9 (persistent template's versions aren't synced with persistent proper's) |
|
Ok cool this is released. The big caveat is: the previous version of persistent-template depends on 2.11, which isn't release, so this isn't available yet in practice |
…or keys
This closes #1104
If people are amenable to this PR, thoughts on the version for it? This could be a breaking change, as described in the Changelog (idk how people feel about that though—if changing the content of a Show/Read instance is considered to be a breaking change)
Before submitting your PR, check that you've:
@sincedeclarations to the Haddock (n/a)After submitting your PR:
(unreleased)on the Changelog