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

Fix EnumNaming textLocation #1977

Merged
merged 1 commit into from
Oct 10, 2019
Merged

Fix EnumNaming textLocation #1977

merged 1 commit into from
Oct 10, 2019

Conversation

BraisGabin
Copy link
Member

@BraisGabin BraisGabin commented Oct 4, 2019

I'm finding some problems with the textLocation thanks to #1975. This is one of them.

I added two tests becase my first try to fix this bug broke that other part.

@@ -30,7 +30,7 @@ class EnumNaming(config: Config = Config.empty) : Rule(config) {
if (!enumEntry.identifierName().matches(enumEntryPattern)) {
report(CodeSmell(
issue,
Entity.from(enumEntry),
Entity.from(enumEntry.identifyingElement!!),
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why the enumEntry should report the wrong text location.
Could you please explain that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. If I use enumEntry the selected code is _Default, (I'm using te test as an example). But we don't want to select the , for that reason I'm using identifyingElement. But, as I said, I don't know this API and I can't find the documentation so I can't be sure about it.

Copy link
Member

@schalkms schalkms Oct 4, 2019

Choose a reason for hiding this comment

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

Honestly, this is weird. However, originalElement did the trick for me.

Copy link
Member Author

Choose a reason for hiding this comment

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

With enumEntry.originalElement it doesn't pass the test for me (it has the same problem as enumEntry)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I also tried it out with different properties. None of them worked.
The problem with identifyingElement is that this API is experimental and I don't really want to have this in production code.
Furthermore, this would also potentially break detekt's baseline feature for this rule.
Unfortunately, I have to ask you to revert this line and adapt the test case. However, I'd really like to have the added tests in the productive code, since we are in the process of adding more tests like these.

I know that reporting the enum including the separator , feels weird, but it's not the end of the world.

Copy link
Member Author

Choose a reason for hiding this comment

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

If the problem is the experimental we could use nameIdentifier. It works too and it's stable. But as I said, without the documentation I don't know the difference between them

Anyway, I checked the baseline problem and you are right. This breaks the previous generated baselines.

Related with the tests: Do you mean to add this test as "Ignored" or something like this? To note that this is wrong but we don't change it for the breaking change? Or adding a test that assume that adding the , is ok?

In the first case I can refactor this PR to do that. If it's the second I think that an issue to talk about how we want the rule test before doing the changes would be great. I'm open to help with that but what should we test? Number of findings? sourcePostion? textPosition?...

Copy link
Member

Choose a reason for hiding this comment

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

If the problem is the experimental we could use nameIdentifier. It works too and it's stable.

I'd go this route.

enum class WorkFlow {
_Default,
}"""
assertThat(NamingRules().compileAndLint(code)[0].charPosition).isEqualTo(TextLocation(26, 34))
Copy link
Member

Choose a reason for hiding this comment

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

This could be made simpler by using the following code snippet.

val findings = NamingRules().compileAndLint(code)
val source = findings.first.location.source
source.line
source.column

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 just find the FindingsAssert class. I'll add a function there and use it.

val code = """
class C(val PARAM: String)
"""
assertThat(NamingRules().lint(code)[0].charPosition).isEqualTo(TextLocation(8, 25))
Copy link
Member

Choose a reason for hiding this comment

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

Same here

@@ -56,6 +57,21 @@ class FindingsAssert(actual: List<Finding>) :
failWithMessage("Expected source locations to be ${expectedSources.toList()} but was ${actualSources.toList()}")
}
}

fun hasTextLocation(vararg expected: TextLocation) = apply {
Copy link
Member

Choose a reason for hiding this comment

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

The helper function is a good idea.

Copy link
Member

@schalkms schalkms Oct 4, 2019

Choose a reason for hiding this comment

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

However, it can be inlined with just 2 asserts on the
source.line
source.column
as we've done it in all other tests.
This refactoring with a helper function should then maybe done globally for all rules-tests, but not in this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@codecov-io
Copy link

codecov-io commented Oct 4, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@8daf49e). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1977   +/-   ##
=========================================
  Coverage          ?   80.75%           
  Complexity        ?     1966           
=========================================
  Files             ?      328           
  Lines             ?     5549           
  Branches          ?     1010           
=========================================
  Hits              ?     4481           
  Misses            ?      536           
  Partials          ?      532
Impacted Files Coverage Δ Complexity Δ
...itlab/arturbosch/detekt/rules/naming/EnumNaming.kt 100% <100%> (ø) 4 <0> (?)

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 8daf49e...995006a. Read the comment docs.

@BraisGabin BraisGabin requested a review from schalkms October 4, 2019 20:57
This was referenced Oct 4, 2019
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.

Please see my last comment in EnumNaming.kt.
Please also use force push only if it's really necessary.
This really eases the review for us. Thank you! 🙂

@@ -30,7 +30,7 @@ class EnumNaming(config: Config = Config.empty) : Rule(config) {
if (!enumEntry.identifierName().matches(enumEntryPattern)) {
report(CodeSmell(
issue,
Entity.from(enumEntry),
Entity.from(enumEntry.identifyingElement!!),
Copy link
Member

Choose a reason for hiding this comment

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

Yes, I also tried it out with different properties. None of them worked.
The problem with identifyingElement is that this API is experimental and I don't really want to have this in production code.
Furthermore, this would also potentially break detekt's baseline feature for this rule.
Unfortunately, I have to ask you to revert this line and adapt the test case. However, I'd really like to have the added tests in the productive code, since we are in the process of adding more tests like these.

I know that reporting the enum including the separator , feels weird, but it's not the end of the world.

@BraisGabin BraisGabin requested a review from schalkms October 6, 2019 11:47
@schalkms schalkms requested a review from arturbosch October 6, 2019 20:23
@BraisGabin
Copy link
Member Author

Ok, so now I'm not using an unstable api. And I remove the !! opetator. If enumEntry.nameIdentifier return null we fallback to enumEntry.

Anyway we have the problem of breaking the baselines...

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.

Thanks!
I think this is a nice addition for v1.2.0
I’m waiting for Artur’s review.

@arturbosch arturbosch merged commit ed7b17d into detekt:master Oct 10, 2019
@arturbosch arturbosch added this to the 1.2.0 milestone Oct 10, 2019
@BraisGabin BraisGabin deleted the enum-naming branch October 24, 2019 14:15
smyachenkov pushed a commit to smyachenkov/detekt that referenced this pull request Dec 9, 2019
smyachenkov pushed a commit to smyachenkov/detekt that referenced this pull request Dec 9, 2019
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.

4 participants