autodocodec icon indicating copy to clipboard operation
autodocodec copied to clipboard

Add HasCodec Const instance and fix HasCodec Identity instance

Open clintonmead opened this issue 1 year ago • 8 comments

clintonmead avatar Aug 01 '23 01:08 clintonmead

Ha! The one time I merge something without demanding tests, it breaks something. Serves me right.

The question now is whether I should try that again or demand tests this time. Last time I hadn't because it required contributions to genvalidity as well.

NorfairKing avatar Aug 01 '23 05:08 NorfairKing

Haha yeah, maybe I should have had tests. It's a subtle bug the typechecker doesn't pick up.

You may want to destroy 0.2.0.4

I've been adding a few more instances also. Perhaps I should have held off with these PRs until I actually tested things to ensure they worked.

clintonmead avatar Aug 01 '23 06:08 clintonmead

In the interest of fixing the bug asap, we can merge these without tests. However, I want to do so only on the condition that we do add these tests later. It does require adding some instances to genvalidity here: https://github.com/NorfairKing/validity/blob/master/genvalidity/src/Data/GenValidity.hs

Does that sound acceptable?

NorfairKing avatar Aug 01 '23 07:08 NorfairKing

Perhaps you should just do the fix for identity. In hindsight there's a heap of instances I want to add (which I've just pushed to this PR).

Perhaps keep this PR open and I'll add tests when I can and we can merge all these instances in one go.

clintonmead avatar Aug 01 '23 08:08 clintonmead

@clintonmead Could you make a separate PR with the fix for identity? (and tests, ideally, they are in autodocodec-api-usage. Almost every Spec file could have an Identity test added)

NorfairKing avatar Aug 08 '23 09:08 NorfairKing

Ping about these. A separate PR is not needed but tests definitely are.

EDIT: I just realised this will require a PR for https://github.com/NorfairKing/validity as well.

NorfairKing avatar Oct 06 '23 14:10 NorfairKing

@clintonmead ping

NorfairKing avatar Nov 10 '23 13:11 NorfairKing

@clintonmead ping.

NorfairKing avatar Nov 21 '23 21:11 NorfairKing