Mismatch for Conway predicate failures

[aniketd]

Description

This is the second (Conway) part of #4619.

Checklist

• Commit sequence broadly makes sense and commits have useful messages
• New tests are added if needed and existing tests are updated
• All visible changes are prepended to the latest section of a CHANGELOG.md for the affected packages.
  New section is never added with the code changes. (See RELEASING.md)
• When applicable, versions are updated in .cabal and CHANGELOG.md files according to the
  versioning process.
• The version bounds in .cabal files for all affected packages are updated.
  If you change the bounds in a cabal file, that package itself must have a version increase. (See RELEASING.md)
• Code is formatted with fourmolu (use scripts/fourmolize.sh)
• Cabal files are formatted (use scripts/cabal-format.sh)
• hie.yaml has been updated (use scripts/gen-hie.sh)
• Self-reviewed the diff

[neilmayhew]

Very nice improvement! Your handling of the protocol encoding variations is neat, too.

My suggestions are mostly cosmetic.

However, there's a more substantial one at the end. It doesn't affect the functionality, but it does affect the readability.

I've also suggested a few additional places where Mismatch could be used.

[lehins]

All my comments in BBODY rule module apply to all other rules affected by this PR.

[TimSheard]

OK I only commented on the Coders parts.
The extra constraint should not be there. We should try hrd to get rid of it.

[lehins]

This is looking great!
A few more simplifications and improvements and it will ready to go

[teodanciu]

I think this requires 1.5.0.0, because it's a breaking change .

[aniketd]

The change won't break any client usage of the library, because it just introduces a new constructor. So a minor bump should be enough I thought.

[lehins]

Suggested change

	version: 1.4.1.0
	version: 1.5.0.0

@teodanciu is right! This is a major breaking change. This is not explicitly listed in PVP, but it is widely considered to be a breaking change in the Haskell community. See some discussions here and comment by Phadej: "It adds constructors to LicenseId and LicenseListVersion, Language.Haskell.Extension.Extension. These are breaking changes."

Here is my reason depicted with an example why I consider such change to be a breaking change:

Imagine package A-1.0.0 defines Foo:

data Foo = Foo | Bar deriving Read

Package B-1.0.0 depends on A >= 1.0 and provides a function:

readFooIxMaybe :: String -> Maybe Int
readFooIxMaybe str = do
  foo <- readMaybe str
  pure $ case foo of
    Foo -> 1
    Bar -> 2

If you add a constructor to Foo and only bump a minor version in package A-1.0.1:

data Foo = Foo | Bar | Baz deriving Read

Then package B-1.0.0 will have the readFooIxMaybe function turn from a total to a partial function.

[lehins]

There is one change that is still necessary that I overlooked and @teodanciu caught. It needs to be fixed. Otherwise it is ready to be merged.

[lehins]

Sorry, I did not catch it earlier, but adding a new constructor to a sum type is commonly considered a breaking change.

Suggested change

	## 1.4.1.0
	## 1.5.0.0

[lehins]

Suggested change

	version: 1.4.1.0
	version: 1.5.0.0

@teodanciu is right! This is a major breaking change. This is not explicitly listed in PVP, but it is widely considered to be a breaking change in the Haskell community. See some discussions here and comment by Phadej: "It adds constructors to LicenseId and LicenseListVersion, Language.Haskell.Extension.Extension. These are breaking changes."

Here is my reason depicted with an example why I consider such change to be a breaking change:

Imagine package A-1.0.0 defines Foo:

data Foo = Foo | Bar deriving Read

Package B-1.0.0 depends on A >= 1.0 and provides a function:

readFooIxMaybe :: String -> Maybe Int
readFooIxMaybe str = do
  foo <- readMaybe str
  pure $ case foo of
    Foo -> 1
    Bar -> 2

If you add a constructor to Foo and only bump a minor version in package A-1.0.1:

data Foo = Foo | Bar | Baz deriving Read

Then package B-1.0.0 will have the readFooIxMaybe function turn from a total to a partial function.

[lehins]

Suggested change

	* Add `{Enc|Dec}CBORGroup` instance for `(a, b)`