Skip to content

Conversation

igfoo
Copy link
Contributor

@igfoo igfoo commented Oct 19, 2022

We were giving warnings about comments, when we were actually trying to populate numlines.

We were giving warnings about comments, when we were actually trying to
populate numlines.
@igfoo igfoo requested a review from a team as a code owner October 19, 2022 17:03
smowton
smowton previously approved these changes Oct 19, 2022
Copy link
Contributor

@smowton smowton left a comment

Choose a reason for hiding this comment

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

Looks sensible; one suggestion for brevity

Comment on lines 19 to 24
val psi2Ir = getPsi2Ir(logger)
init {
if (psi2Ir == null) {
logger.warn("Lines of code will not be populated as Kotlin version is too old (${KotlinCompilerVersion.getVersion()})")
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
val psi2Ir = getPsi2Ir(logger)
init {
if (psi2Ir == null) {
logger.warn("Lines of code will not be populated as Kotlin version is too old (${KotlinCompilerVersion.getVersion()})")
}
}
val psi2Ir = getPsi2Ir(logger) ?: run {
logger.warn("Lines of code will not be populated as Kotlin version is too old (${KotlinCompilerVersion.getVersion()})")
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that this would also need a ; null in the run to get the right result. I've switched it to use .also instead, which I think is nicer than a separate init block. It doesn't get us the concise ?: null check, but I think that it's more natural to "also" do something that doesn't have a result.

@igfoo igfoo requested a review from a team as a code owner October 20, 2022 13:18
@github-actions github-actions bot added the Java label Oct 20, 2022
@igfoo igfoo merged commit 2e6d6e1 into github:main Oct 21, 2022
@igfoo igfoo deleted the igfoo/psi branch October 21, 2022 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants