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

Remove $ as a valid char in class and method names #2662

Merged
merged 4 commits into from
May 9, 2020
Merged

Remove $ as a valid char in class and method names #2662

merged 4 commits into from
May 9, 2020

Conversation

BraisGabin
Copy link
Member

This issue was spotted in #2656 (but it's not completely related with that issue).

Extracted from the issue:

I just realised. Why do we allow classes with $ in the name? It has little sense. The $ is usually used by the annotation processors to create new classes. And they use that symbol to get a name that is not going to conflict with the user class names. Our tests didn't check the use of $ neighter. And you can't use te symbol $ if you don't use the char `. I think that it's safe to remove it.

I can't even write tests for this change because I can't create a name of a class with $ if I'm not using . And the use of in class names is exactly what is being talked in #2656. Right now we don't allow it.

@codecov
Copy link

codecov bot commented May 8, 2020

Codecov Report

Merging #2662 into master will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2662      +/-   ##
============================================
- Coverage     80.00%   79.98%   -0.02%     
- Complexity     2292     2303      +11     
============================================
  Files           379      380       +1     
  Lines          6761     6770       +9     
  Branches       1222     1224       +2     
============================================
+ Hits           5409     5415       +6     
  Misses          740      740              
- Partials        612      615       +3     
Impacted Files Coverage Δ Complexity Δ
...tlab/arturbosch/detekt/rules/naming/ClassNaming.kt 100.00% <100.00%> (ø) 4.00 <0.00> (ø)
...b/arturbosch/detekt/rules/naming/FunctionNaming.kt 94.73% <100.00%> (ø) 9.00 <0.00> (ø)
...ules/style/optional/MandatoryBracesIfStatements.kt 64.28% <0.00%> (-7.94%) 14.00% <0.00%> (ø%)
...bosch/detekt/rules/providers/StyleGuideProvider.kt 100.00% <0.00%> (ø) 2.00% <0.00%> (ø%)
...etekt/rules/style/optional/MandatoryBracesLoops.kt 75.00% <0.00%> (ø) 11.00% <0.00%> (?%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a1d8391...dbc6b6a. Read the comment docs.

Copy link
Member

@schalkms schalkms left a comment

Choose a reason for hiding this comment

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

Shouldn’t this be adapted for functions an other elements as well?

@BraisGabin
Copy link
Member Author

I didn't think of that. Yeah, I'm going to check if we have the same bug in the other ones

@BraisGabin BraisGabin changed the title Fix ClassNaming pattern Remove $ as a valid symbol in class and method names May 9, 2020
@BraisGabin BraisGabin requested a review from schalkms May 9, 2020 13:44
@BraisGabin
Copy link
Member Author

Updated.

Copy link
Member

@schalkms schalkms left a comment

Choose a reason for hiding this comment

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

Can variables and parameters also have `` in the name?

@BraisGabin
Copy link
Member Author

Can variables and parameters also have `` in the name?

I think that I'm missing something. This PR is not related with the use of . It just removes $` as a valid char in the function and method names.

In the parameters and variables we wasn't allowing that char so there's nothing to change there.

@BraisGabin BraisGabin changed the title Remove $ as a valid symbol in class and method names Remove $ as a valid char in class and method names May 9, 2020
@BraisGabin BraisGabin requested a review from schalkms May 9, 2020 13:54
@schalkms
Copy link
Member

schalkms commented May 9, 2020

Thanks for the clarification! @BraisGabin

@schalkms schalkms merged commit 924104e into detekt:master May 9, 2020
@arturbosch arturbosch added this to the 1.8.1 milestone May 10, 2020
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