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

Bug (?) in atom_naming_convention #312

Closed
paulo-ferraz-oliveira opened this issue Mar 17, 2023 · 6 comments · Fixed by #356
Closed

Bug (?) in atom_naming_convention #312

paulo-ferraz-oliveira opened this issue Mar 17, 2023 · 6 comments · Fixed by #356
Labels
Milestone

Comments

@paulo-ferraz-oliveira
Copy link
Collaborator

Bug Description

atom_naming_convention might be wrongly implemented (the default regex part).

I recently noticed non200_ was a possible atom / function_name, but this is probably undesired.

Now, if we fix it as per our initial intention (which was...?), it is a breaking change, so we'd need to bump major.

To Reproduce

Have non200_ in your code as an atom, or function name.

Expected Behavior

The regex should be improved, but also... why would non_200 not be accepted?

@elbrujohalcon
Copy link
Member

elbrujohalcon commented Mar 17, 2023

non_200 should be accepted. Let's improve the regex

@paulo-ferraz-oliveira
Copy link
Collaborator Author

Yeah, tentatively I'm leaving this here: ^([a-z][a-z0-9]*(?:_[a-z0-9]+)*)(?:_SUITE)?$.

We force:

  • starting with [a-z]
  • potentially following with 0 or more [a-z0-9]
  • potentially following with 0 or more "_ forcefully followed by [a-z0-9]"
  • eventually ending in _SUITE

@bormilan
Copy link
Contributor

Is this issue still relevant? Because I tested with the given atom name(non200_) and it passed, the regex is already updated as I can see.

the default regex: "^([a-z][a-z0-9]_?)(_SUITE)?$"

I think it's correct that it allows atoms like non200_.

@elbrujohalcon
Copy link
Member

elbrujohalcon commented Sep 11, 2024

@bormilan The original idea was not to allow atoms like non200_ but instead allow atoms like non_200. An atom that ends with an underscore is, at the very least, suspicious. I would like Elvis to emit a warning for it. Users can always change the default regex to whatever they like, anyway, if non200_ makes sense for their use case.

@bormilan
Copy link
Contributor

Oh, I completely misunderstood. So we want to warn the developers to not make atoms like non200_ while allowing atoms like non_200 at the same time.

@elbrujohalcon
Copy link
Member

Exactly! 👌🏻

bormilan pushed a commit to bormilan/elvis_core that referenced this issue Sep 11, 2024
bormilan pushed a commit to bormilan/elvis_core that referenced this issue Sep 11, 2024
@elbrujohalcon elbrujohalcon added this to the 4.0.0 milestone Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants