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

feat(#297): New rule: Prefer Unquoted Atoms #355

Merged
merged 8 commits into from
Sep 16, 2024

Conversation

bormilan
Copy link
Contributor

@bormilan bormilan commented Sep 3, 2024

This is in an early stage, I have som things to fix.

Copy link
Member

@elbrujohalcon elbrujohalcon left a comment

Choose a reason for hiding this comment

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

The code is fine, I would just simplify it by removing the options.

src/elvis_style.erl Outdated Show resolved Hide resolved
src/elvis_style.erl Outdated Show resolved Hide resolved
src/elvis_style.erl Outdated Show resolved Hide resolved
@elbrujohalcon
Copy link
Member

Once you're done with the code, @bormilan … you also need to add docs to this rule (in RULES.md) and you need to include it in the appropriate ruleset(s) (I guess erl_files and hrl_files)

@bormilan
Copy link
Contributor Author

bormilan commented Sep 4, 2024

Thanks for all the help and suggestions, currently I'm on a little vacation, but after that I will fix all of them and update the PR.

@bormilan
Copy link
Contributor Author

Today I made some changes, but I had some questions that I commented on your suggestions above.
Besides these, I have made two hardcoded variables that I want to put somewhere else(bc they are constant values), but I don't know where to. If you could help me with that I would appreciate it. These are Regex and KeyWords.

Copy link
Member

@elbrujohalcon elbrujohalcon left a comment

Choose a reason for hiding this comment

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

A few improvements and… I wonder what happens with 'maybe' as an atom.
I checked erlang/otp's code and wherever there is a list of keywords/reserved words… they do not include maybe, but I guess… if the feature is enabled (and I think it's enabled by default on OTP27+), that one should also be considered a keyword, right?

doc_rules/elvis_style/prefer_unquoted_atoms.md Outdated Show resolved Hide resolved
src/elvis_style.erl Outdated Show resolved Hide resolved
src/elvis_style.erl Outdated Show resolved Hide resolved
test/examples/pass_unquoted_atoms.erl Outdated Show resolved Hide resolved
Copy link
Member

@elbrujohalcon elbrujohalcon left a comment

Choose a reason for hiding this comment

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

The code is perfectly fine, @bormilan … but I still believe this rule belongs to elvis_text_style and not elvis_style.

@bormilan
Copy link
Contributor Author

So, should we add "maybe" to the exceptions? I think we should. But because now it's not a keyword by default, we may wait and add it when erl/OTP27 comes.

@elbrujohalcon
Copy link
Member

Personally… I would add it right now… It will be a reserved word soon enough.
I wish Erlang/OTP would have a public function that returned the list of reserved words somewhere (erl_syntax may be a great place)… 🤷🏻

RULES.md Outdated Show resolved Hide resolved
doc_rules/elvis_style/prefer_unquoted_atoms.md Outdated Show resolved Hide resolved
@elbrujohalcon
Copy link
Member

Excellent work, @bormilan !!! Thank you very much!

@bormilan
Copy link
Contributor Author

Thank you so much for all the help and patience. It was a great start to my journey.

@bormilan
Copy link
Contributor Author

I found a little problem, the formatter changes the 'maybe' to maybe in the list of exceptions. Should I turn it off to that file, or we should consider changing this in the formatter itself to make an exception for it?

@elbrujohalcon
Copy link
Member

@bormilan The list of exceptions should have strings and not atoms… that would fix it :)

@elbrujohalcon
Copy link
Member

But… the formatter is broken, anyway… please report an issue there.

@bormilan
Copy link
Contributor Author

Personally… I would add it right now… It will be a reserved word soon enough. I wish Erlang/OTP would have a public function that returned the list of reserved words somewhere (erl_syntax may be a great place)… 🤷🏻

I think f_reserved_word does exactly what you want in the module erl_scan

@elbrujohalcon
Copy link
Member

Personally… I would add it right now… It will be a reserved word soon enough. I wish Erlang/OTP would have a public function that returned the list of reserved words somewhere (erl_syntax may be a great place)… 🤷🏻

I think f_reserved_word does exactly what you want in the module erl_scan

Yeah, but it's not exported, is it?

@elbrujohalcon elbrujohalcon added this to the 4.0.0 milestone Sep 16, 2024
@elbrujohalcon elbrujohalcon merged commit ae6258d into inaka:main Sep 16, 2024
9 checks passed
@bormilan
Copy link
Contributor Author

Personally… I would add it right now… It will be a reserved word soon enough. I wish Erlang/OTP would have a public function that returned the list of reserved words somewhere (erl_syntax may be a great place)… 🤷🏻

I think f_reserved_word does exactly what you want in the module erl_scan

Yeah, but it's not exported, is it?

It is:
image

@elbrujohalcon
Copy link
Member

elbrujohalcon commented Sep 16, 2024

Personally… I would add it right now… It will be a reserved word soon enough. I wish Erlang/OTP would have a public function that returned the list of reserved words somewhere (erl_syntax may be a great place)… 🤷🏻

I think f_reserved_word does exactly what you want in the module erl_scan

Yeah, but it's not exported, is it?

It is: image

Ah, right! I was thinking of something like f_reserved_words/0 (that would return the list) :)

But… maybe we can actually use that here or in rebar3_format… 🤔
If you want to send a PR using erl_scan:f_reserved_word/1 to verify that the atom is actually a reserved word instead of the hardcoded list of strings we now have… I'd merge it, for sure!

@bormilan
Copy link
Contributor Author

Personally… I would add it right now… It will be a reserved word soon enough. I wish Erlang/OTP would have a public function that returned the list of reserved words somewhere (erl_syntax may be a great place)… 🤷🏻

I think f_reserved_word does exactly what you want in the module erl_scan

Yeah, but it's not exported, is it?

It is: image

Ah, right! I was thinking of something like f_reserved_words/0 (that would return the list) :)

But… maybe we can actually use that here or in rebar3_format… 🤔 If you want to send a PR using erl_scan:f_reserved_word/1 to verify that the atom is actually a reserved word instead of the hardcoded list of strings we now have… I'd merge it, for sure!

Ok, I will do it shortly!

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.

3 participants