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

Warn on unnecessary quotes #7838

Merged
merged 4 commits into from
Jul 10, 2018
Merged

Warn on unnecessary quotes #7838

merged 4 commits into from
Jul 10, 2018

Conversation

josevalim
Copy link
Member

Closes #7634

@josevalim
Copy link
Member Author

Also, should it be :warn_on_unnecessary_quote or :warn_on_unnecessary_quotes? I went with the second but I thought I would double check.

@@ -650,6 +650,10 @@ defmodule Code do
when non-existing atoms are found by the tokenizer.
Defaults to `false`.

* `:warn_on_unnecessary_quotes` - when `false`, does not warn
when atoms, keywords and calls have unnecessary quotes on
them
Copy link
Member

Choose a reason for hiding this comment

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

I would add a dot at the end and the default value to keep consistency with previous descriptions.

  • :warn_on_unnecessary_quotes - when false, does not warn when atoms, keywords and calls have unnecessary quotes on them. Defaults to true.

Copy link
Member

@fertapric fertapric Jul 7, 2018

Choose a reason for hiding this comment

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

And maybe use "or" instead of "and"?

does not warn when atoms, keywords or calls have unnecessary quotes on them

Copy link
Member

@michalmuskala michalmuskala left a comment

Choose a reason for hiding this comment

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

I like the feature and I also think warn_on_unnecessary_quotes is better.

@@ -650,6 +650,10 @@ defmodule Code do
when non-existing atoms are found by the tokenizer.
Defaults to `false`.

* `:warn_on_unnecessary_quotes` - when `false`, does not warn
when atoms, keywords or calls have unnecessary quotes on
them.
Copy link
Member

Choose a reason for hiding this comment

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

I think the documentation about the default value is handy :)

I just want to be sure that part was not overlooked from my previous comments. If you disagree, no worries :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I missed it. :) Tks for double checking.

Copy link
Member

@whatyouhide whatyouhide left a comment

Choose a reason for hiding this comment

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

👏

@josevalim josevalim merged commit beb596d into master Jul 10, 2018
@josevalim
Copy link
Member Author

❤️ 💚 💙 💛 💜

@garazdawi
Copy link

I've been looking at barrel-db/rebar3_elixir_compile#37 and noticed that you no longer use Code.eval_string in Mix.Dep.Lock. From this I assume that it is intentional that you cannot turn off the warning when using Code.eval_string? So the rebar3 plugin should to the same thing?

@josevalim
Copy link
Member Author

Yes, the rebar3 plugin should do the same. The options given to eval are not forwarded to string_to_quoted but you can just call it directly and put it all together. This approach should also be backwards compatibility. string_to_quoted and eval_quoted have been there forever!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants