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

Simplify internal parsing to KtFile's #2875

Merged
merged 2 commits into from
Jul 20, 2020
Merged

Simplify internal parsing to KtFile's #2875

merged 2 commits into from
Jul 20, 2020

Conversation

arturbosch
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented Jul 15, 2020

Codecov Report

Merging #2875 into master will decrease coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2875      +/-   ##
============================================
- Coverage     80.28%   80.24%   -0.04%     
+ Complexity     2417     2414       -3     
============================================
  Files           421      421              
  Lines          7359     7356       -3     
  Branches       1346     1345       -1     
============================================
- Hits           5908     5903       -5     
  Misses          756      756              
- Partials        695      697       +2     
Impacted Files Coverage Δ Complexity Δ
.../main/kotlin/io/github/detekt/parser/KtCompiler.kt 86.95% <100.00%> (-5.36%) 7.00 <1.00> (-2.00)
...lab/arturbosch/detekt/formatting/FormattingRule.kt 89.65% <0.00%> (-3.45%) 12.00% <0.00%> (-1.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 828d65d...27b5417. Read the comment docs.

@arturbosch arturbosch marked this pull request as ready for review July 15, 2020 20:37
Comment on lines +41 to +48
val errors = mutableListOf<PsiErrorElement>()
ktFile.accept(object : KtTreeVisitorVoid() {
override fun visitErrorElement(element: PsiErrorElement) {
errors.add(element)
}
})

assertThat(errors).isNotEmpty()
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 a behavioral change now, since you had to adapt the test case?

Copy link
Member Author

@arturbosch arturbosch Jul 15, 2020

Choose a reason for hiding this comment

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

Hm, yes it is. Not sure how to handle this.
In KtTreeCompiler we throw an exception if the file ending is not kt or kts.

In the old implementation the PsiFactory tried to use Language.Any and because no language was registered was .css it didn't tried to parse anything and returned null which we checked with error(...).
The KtPsiFactory always tries to parse Kotlin and returns a PsiFile with errors.

@schalkms @BraisGabin @cortinico should we throw an error on wrong file extension like when parsing directories?

private fun Path.isKotlinFile(): Boolean {
val fullPath = toAbsolutePath().toString()
val kotlinEnding = fullPath.substring(fullPath.lastIndexOf('.') + 1)
return kotlinEnding in KT_ENDINGS
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, we ignored it when parsing many files:

settings.info("Ignoring a file detekt cannot handle: $path")

@arturbosch arturbosch added this to the 1.11.0 milestone Jul 20, 2020
@arturbosch arturbosch added the housekeeping Marker for housekeeping tasks and refactorings label Jul 20, 2020
@arturbosch arturbosch merged commit 9874528 into master Jul 20, 2020
@arturbosch arturbosch deleted the simplify-parsing branch July 20, 2020 17:17
@arturbosch arturbosch modified the milestones: 1.11.0, 1.10.1 Aug 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
housekeeping Marker for housekeeping tasks and refactorings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants