-
Notifications
You must be signed in to change notification settings - Fork 90
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
#164 - Support for Elm, level 1: Tuples, Lists & Records #168
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work! I can't comment on elm specifics, but you seem to have a solid test suite and an implementation that looks very reasonable. I've left some requests for changes mostly on naming and stylistic tweaks. Let me know what you think.
Thanks @AndrewRadev, all good suggestions :) I'll wave when my PR includes them. |
@AndrewRadev there it is. Don't hesitate to point out whatever else you might find :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me 👍. Will go ahead and merge.
Great, thanks a lot for all your help :) can't wait to upgrade and use it now :) |
As promised in issue #164, here we are, the first PR :)
Don't hesitate to point out if you see something I forgot or there's a canonical function I accidentally re-invented, but it seems to do the job and it's quite extensively tested for most problematic cases I could come up with.