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

Normalize all Elm keywords #41

Merged

Conversation

madsflensted
Copy link

Here is an update the normalizes all the Elm keywords, as listed in the Elm compilers Parser code.

I updated the unit tests to test all keywords, but I am not sure if I like this style, where the test and the code basically share the same data, and therefor testing the whole list bring little extra value over testing a single entry.

@dillonkearns
Copy link
Owner

@madsflensted fantastic, thank you so much for the PR! That’s fantastic that you linked to the Elm compiler section that includes the list of keywords 💯!

I won’t be at a computer until around Feb. 24 so I won’t be able to do a new release before then. I’ll review this a little more in-depth then. One small thing, though, would you mind running elm-format version 0.7.0-exp on this code to avoid whitespace diff noise?

@madsflensted
Copy link
Author

@dillonkearns yes totally agree that was not a very nice PR diff-wize. I have now used the right elm-format version, and also changed the test code to allow for a more precise diff.

No pressure on the release, it is an easy thing to work around for me since I control the server side schema.

Thank you for a great library! I have stumbled on a few mental bumps getting started, I will try to share them on Slack when I have a bit of time.

@dillonkearns
Copy link
Owner

Hey @madsflensted thanks again for taking the lead on this! I really appreciate it 😄

I did a little experimentation and it appears that several of these are valid top-level identifiers. In particular, alias, left, right, non, effect, command, subscription, true, false, null. Here's an Ellie example that demonstrates that. Do you see any issue with treating these like any other word given that they compile as top-level identifiers?

@dillonkearns
Copy link
Owner

@madsflensted so it looks like some of these keywords are not reserved words, see Variable.hs#L64-L75. infix is the exception, it isn't listed as a reserved word (only a keyword, see https://en.wikipedia.org/wiki/Reserved_word), but the compiler gets confused when it is used as a top-level identifier so I added it to the list as well.

@dillonkearns dillonkearns merged commit 1a092d0 into dillonkearns:master Mar 2, 2018
@dillonkearns
Copy link
Owner

I went ahead and merged it in, thanks again for the PR @madsflensted!

I agree with your point about the testing style, this article has a good way of looking at that idea (tl;dr: it's okay for your tests to have duplication, in fact if they don't then sometimes it's hard to be confident in them!): http://arlobelshee.com/wet-when-dry-doesnt-apply/. I removed the elmReservedWords from the public interface and added a test of just a subset of the keywords. You can see some of the commits if you're interested: dillonkearns/graphqelm@1a092d0...madsflensted-normalize-all-keywords

Thanks for the help! Let me know if you think anything here could be improved.

@madsflensted
Copy link
Author

Sounds like some good choices, and thank you for the linked article.
Should normalization be mentioned in the Fields docs? or somewhere else?

@dillonkearns
Copy link
Owner

Should normalization be mentioned in the Fields docs? or somewhere else?

That's a great question, it does seem like that would be good to have it documented somewhere. I wonder if the README would be more accessible? And I could link to it in the field docs perhaps.

@dillonkearns
Copy link
Owner

@madsflensted I added a brief description of the normalization in the FAQ, and linked to the FAQ from the README.

Thanks again for your input on this!

@madsflensted
Copy link
Author

Looking good! Keep up the great work on this package.

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.

None yet

3 participants