Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for miniBill/codec #13

Merged
merged 8 commits into from
May 13, 2024
Merged

Add support for miniBill/codec #13

merged 8 commits into from
May 13, 2024

Conversation

MartinSStewart
Copy link
Collaborator

@MartinSStewart MartinSStewart commented May 8, 2024

@miniBill you can now generate your codecs with elm-review-todo-it-for-me!

@gampleman
Copy link
Owner

gampleman commented May 8, 2024

Looks good but I would be happier if there was a test (even something simple like FuzzerCodeGenTest.elm). See the docs for fakeDependency on how to generate that value. And also please update the README as well :)

Repository owner deleted a comment from jakub-nlx May 8, 2024
@MartinSStewart
Copy link
Collaborator Author

Oops, yes got excited and forgot the boring details. Will fix in a moment.

@MartinSStewart
Copy link
Collaborator Author

I copied the elm-serialize tests and changed them to fit elm-codec. Only two tests are still failing. For some reason Maybe isn't being detected as a core type and code gen is generating a maybeCodec instead of using Codec.nullable.

README.md Show resolved Hide resolved
, CodeGenerator.float (val "float")
, CodeGenerator.string (val "string")
, CodeGenerator.list (fn1 "list")
, CodeGenerator.maybe (fn1 "nullable")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
, CodeGenerator.maybe (fn1 "nullable")
, CodeGenerator.use "Codec.nullable"

This might fix the Maybe related issues. This is an annoying workaround, but I haven't quite figured out how to fix this issue properly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that but it doesn't seem to work either. The thing is, I first used Codec.maybe which wasn't correct but ought to have worked for code gen but didn't for some reason. I'm confused what difference there could be that causes elm-serialize to correctly handle Maybe but not elm-codec.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I’ll try to find some time on Monday to look into it a bit more.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I fixed this, it turned out there was a stupid oversight in the code that was managing this stuff which just happened to work for the other use case because of different source order :(

@gampleman gampleman merged commit e26957a into master May 13, 2024
2 checks passed
@gampleman gampleman deleted the miniBillCodec branch May 13, 2024 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants