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

Bugfix in the LineLength rule #929

Merged
merged 8 commits into from
Jun 16, 2021
Merged

Bugfix in the LineLength rule #929

merged 8 commits into from
Jun 16, 2021

Conversation

kgevorkyan
Copy link
Member

What's done:

  • Fix bug with NPE in long comments in case when comment located at the line length limit
  • Add tests

Explanation:
When we looking for the node, which code len is greater than limit, we should keep in mind, that
if limit = 120, it means, that the 119 symbol, i.e. 119 element from code line array exceeds the limit.

But previously, in

val newNode = node.psi.findElementAt(offset + configuration.lineLength.toInt())!!.node

we took the 120 element, which offset is actually 121 and so NPE appeared in

val indexLastSpace = wrongNode.text.substring(0, configuration.lineLength.toInt() - leftOffset).lastIndexOf(' ')

since configuration.lineLength.toInt() == 120 and leftOffset == 121

### What's done:
* WIP
### What's done:
* few fixes
### What's done:
* Fix bug with NPE in comments
* WIP: bug with npe in string templates
### What's done:
* polish
@kgevorkyan kgevorkyan requested a review from orchestr7 June 16, 2021 09:50
@kgevorkyan kgevorkyan linked an issue Jun 16, 2021 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Jun 16, 2021

Codecov Report

Merging #929 (a5e92e3) into master (6ee9d43) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #929   +/-   ##
=========================================
  Coverage     84.14%   84.14%           
  Complexity     2373     2373           
=========================================
  Files           101      101           
  Lines          6006     6006           
  Branches       1772     1772           
=========================================
  Hits           5054     5054           
  Misses          263      263           
  Partials        689      689           
Flag Coverage Δ
unittests 84.14% <100.00%> (ø)

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

Impacted Files Coverage Δ
...g/cqfn/diktat/ruleset/rules/chapter3/LineLength.kt 91.78% <100.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 6ee9d43...a5e92e3. Read the comment docs.

}

private fun validateAccessors(node: ASTNode, propsWithBackSymbol: List<String>) {
val accessors = node.findAllDescendantsWithSpecificType(PROPERTY_ACCESSOR).filter { it.hasChildOfType(BLOCK) } // exclude get with expression body
Copy link
Member

Choose a reason for hiding this comment

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

Is this the line that caused an error? Could you make this example more minimal, e.g. the logic function doesn't have anything to do with line length 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.

Thanks, agree, now this test is more compact

@kgevorkyan kgevorkyan merged commit 4112a2b into master Jun 16, 2021
@kgevorkyan kgevorkyan deleted the bugfix/line_length_bug branch June 16, 2021 12:25
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.

String index out of range: -1 in Line Length Rule
2 participants