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

Rework naming rules to match intellij inspections #569

Closed
wants to merge 5 commits into from

Conversation

arturbosch
Copy link
Member

@arturbosch arturbosch commented Nov 23, 2017

Ok, the serialVersionUID name is now supported by default.

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.

I like the new naming features!

active: true
constantPattern: '^([A-Z_]*|serialVersionUID)$'
propertyPattern: '[A-Za-z][_A-Za-z0-9]*'
TopLevelPropertyNaming:
Copy link
Member

Choose a reason for hiding this comment

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

I like the granularity for the naming rules.


override val issue = Issue(javaClass.simpleName,
Severity.Style,
"Constants names should follow the naming convention set in the projects configuraiton.",
"Constants inside objects should follow the naming convention set in the projects configuration.",
Copy link
Member

Choose a reason for hiding this comment

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

It might be a bit unclear what the convention set looks like for the reader of this message. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I don't get what you mean :)?

message = "Private top level property names should match the pattern: $propertyPattern"))
}
} else {
if (!property.identifierName().matches(propertyPattern)) {
Copy link
Member

Choose a reason for hiding this comment

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

Should detekt's rule configuration really differentiate between private and non-private top level properties?

Copy link
Member Author

Choose a reason for hiding this comment

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

IntelliJ does, so I also implemented this. If having only top level declarations inside a file, it could be handy to name things with _ ^^

import org.assertj.core.api.Assertions
import org.junit.jupiter.api.Test

class NamingConventionCustomPatternTest {
Copy link
Member

Choose a reason for hiding this comment

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

Splitting up the these test cases is a good idea!

// valid: underscore only allowed for private vars
private val _variable = 5
val _variable2 = 5 // invalid

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 all these test cases should be in separate test cases and not in one big file. This could lead to problems discussed in #527

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 think after this PR this file and NamingConventionSpec is not needed anymore, as all cases/rules have their own spec class then ^^

@arturbosch
Copy link
Member Author

Merged manually.

@arturbosch arturbosch closed this Nov 27, 2017
@arturbosch arturbosch deleted the top-level-properties-not-constants branch November 27, 2017 15:53
@arturbosch arturbosch added this to the RC5-6 milestone Nov 27, 2017
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

2 participants