search

NorfairKing / autodocodec

self(auto)- documenting encoders and decoders
MIT License
121 stars 20 forks source link

Add HasCodec Const instance and fix HasCodec Identity instance #44

Closed clintonmead closed 9 months ago

NorfairKing commented 1 year ago

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.

clintonmead commented 1 year ago

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.

NorfairKing commented 1 year ago

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?

clintonmead commented 1 year ago

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.

NorfairKing commented 1 year ago

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

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

@clintonmead ping

NorfairKing commented 1 year ago

@clintonmead ping.