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.

Sign up for GitHub

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

Jump to bottom

Fix bug with incorrect reordering of enum classes #847

Merged
merged 7 commits into from
Apr 26, 2021
Merged

Fix bug with incorrect reordering of enum classes #847

merged 7 commits into from
Apr 26, 2021

Conversation

petertrr
Copy link
Member

@petertrr petertrr commented Apr 26, 2021 •
edited
Loading

What's done:

  • Changed code style guide - properties are considered loggers only if they are private
  • Fixed bug in logic
  • Added tests
  • Added smoke tests
  • Refactoring

This pull request closes #843

@petertrr petertrr force-pushed the bugfix/class-ordering-in-enums#843 branch from 2fb0bea to c643e2d Compare April 26, 2021 07:53
@petertrr
Fix bug with incorrect reordering of enum classes
3b7766a 
### What's done:
* Fixed bug in logic
* Added tests
* Added smoke tests
* Refactoring
@petertrr petertrr force-pushed the bugfix/class-ordering-in-enums#843 branch from c643e2d to 3b7766a Compare April 26, 2021 07:58
@codecov
Copy link

codecov bot commented Apr 26, 2021 •
edited
Loading

Codecov Report

Merging #847 (9ca9deb) into master (60dac69) will decrease coverage by 0.00%.
The diff coverage is 90.32%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #847      +/-   ##
============================================
- Coverage     81.01%   81.00%   -0.01%     
+ Complexity     2292     2282      -10     
============================================
  Files           100      100              
  Lines          5840     5843       +3     
  Branches       1815     1814       -1     
============================================
+ Hits           4731     4733       +2     
  Misses          286      286              
- Partials        823      824       +1
Flag Coverage Δ Complexity Δ
unittests 81.00% <90.32%> (-0.01%) 2282.00 <12.00> (-10.00)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
...set/rules/chapter3/ClassLikeStructuresOrderRule.kt 87.50% <90.32%> (-0.68%) 35.00 <12.00> (-10.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 60dac69...9ca9deb. Read the comment docs.

aktsay6
aktsay6 reviewed Apr 26, 2021
View reviewed changes
...rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter3/ClassLikeStructuresOrderRule.kt Outdated
.allBlockFlattened()
.map { astNode ->
Pair(astNode, astNode.siblings(false).takeWhile { it.elementType == WHITE_SPACE || it.isPartOfComment() }.toList())
listOf(astNode) + astNode.siblings(false).takeWhile { it.elementType == WHITE_SPACE || it.isPartOfComment() }.toList()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to check Annotations in here?

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, annotations are usually part of node with declaration, specifically part of modifier list. Can you give any example that would break with this logic?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, forgot that they are in modifier list. Then everything should be fine

orchestr7
orchestr7 reviewed Apr 26, 2021
View reviewed changes
...rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter3/ClassLikeStructuresOrderRule.kt
@@ -73,49 +75,21 @@ class ClassLikeStructuresOrderRule(configRules: List<RulesConfig>) : DiktatRule(
val initBlocks = node.getChildren(TokenSet.create(CLASS_INITIALIZER)).toMutableList()
val constructors = node.getChildren(TokenSet.create(SECONDARY_CONSTRUCTOR)).toMutableList()
val methods = node.getChildren(TokenSet.create(FUN)).toMutableList()
val (usedClasses, unusedClasses) = node
Copy link
Member

Choose a reason for hiding this comment

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

not able to understand where was the issue because of the refactoring, could you please provide some info?

Copy link
Member Author

Choose a reason for hiding this comment

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

We are explicitly choosing class elements to sort. We were simply skipping enum members, now I added handling of them. I believe there are no more unexpected class elements that are not covered.

@orchestr7
Copy link
Member

allProperties.filter { it.findLeafWithSpecificType(CONST_KEYWORD) != null }.toMutableList()
at least this part can be moved to functions

@orchestr7
Copy link
Member

orchestr7 commented Apr 26, 2021 •
edited
Loading

why do we select different properties? and after that unify them:

 AllProperties(loggers, constProperties, properties, lateInitProperties),

this is useless, no?

@orchestr7
Copy link
Member

node.getChildren(TokenSet.create(FUN)).toMutableList()
this can be moved to a different function

@petertrr
Copy link
Member Author

why do we select different properties? and after that unify them:

 AllProperties(loggers, constProperties, properties, lateInitProperties),

this is useless, no?

We group them in four distinct blocks and then arrange in order of these blocks. I believe I introduced this class so that Blocks wouldn't have too much arguments

petertrr added 2 commits April 26, 2021 15:58
@petertrr
Fix bug with incorrect reordering of enum classes
8e6c7c7 
### What's done:
* Refactoring
* Changed code style guide for logger properties
@petertrr
Merge remote-tracking branch 'origin/bugfix/class-ordering-in-enums#8…
421f040 
…43' into bugfix/class-ordering-in-enums#843
@petertrr petertrr requested a review from orchestr7 April 26, 2021 12:59
@petertrr petertrr force-pushed the bugfix/class-ordering-in-enums#843 branch from 33224ae to 8ee50f1 Compare April 26, 2021 13:03
orchestr7
orchestr7 approved these changes Apr 26, 2021
View reviewed changes
Copy link
Member

@orchestr7 orchestr7 left a comment

Choose a reason for hiding this comment

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

lgtm

@petertrr petertrr merged commit c04b83f into master Apr 26, 2021
@petertrr petertrr deleted the bugfix/class-ordering-in-enums#843 branch April 26, 2021 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aktsay6 aktsay6 aktsay6 left review comments

@orchestr7 orchestr7 orchestr7 approved these changes

@kentr0w kentr0w Awaiting requested review from kentr0w

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

autofix for WRONG_ORDER_IN_CLASS_LIKE_STRUCTURES removes values from enum class
3 participants
@petertrr @orchestr7 @aktsay6