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 Signatures.kt:buildFunctionSignature #4961

Merged
merged 4 commits into from Jun 27, 2022

Conversation

VitalyVPinchuk
Copy link
Contributor

For #4887 ('buildFunctionSignature' fails when function body has KDoc)

'buildFunctionSignature' fails when function body has KDoc.
@VitalyVPinchuk
Copy link
Contributor Author

:detekt-api:test fails, but locally it passes alright. Why is that?

@BraisGabin
Copy link
Member

You can find information about that here: #4874. Any feedback there is more than welcome.

Copy link
Member

@BraisGabin BraisGabin left a comment

Choose a reason for hiding this comment

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

Thanks!

@BraisGabin
Copy link
Member

This is a fix but it's also kind of "breaking". It will force to regenerate lots of baselines. I'm ok with that but just a heads up if some maintainer wants to pospone he merge to 1.22.0. cc @cortinico

@cortinico
Copy link
Member

This is a fix but it's also kind of "breaking". It will force to regenerate lots of baselines. I'm ok with that but just a heads up if some maintainer wants to pospone he merge to 1.22.0

I think we can merge this, but would be great to include a paragraph in the 'migration' section of the changelog.

@BraisGabin BraisGabin added notable changes Marker for notable changes in the changelog migration Marker to add a migration step in the changelog and removed api notable changes Marker for notable changes in the changelog labels Jun 27, 2022
@BraisGabin
Copy link
Member

I think that something like this could do the trick on the changelog:

We updated the baseline creation. Now the kdoc is not part of the function signature. This could create that already suppressed issues in your baseline reappear. To fix that just recreate your baseline.

@BraisGabin BraisGabin merged commit 2b068e0 into detekt:main Jun 27, 2022
@VitalyVPinchuk
Copy link
Contributor Author

I think that something like this could do the trick on the changelog:

We updated the baseline creation. Now the kdoc is not part of the function signature. This could create that already suppressed issues in your baseline reappear. To fix that just recreate your baseline.

I think KDoc comments were trimmed before that too.

element.docComment?.let {
    methodStart = it.endOffset - it.startOffset
}

@cortinico cortinico added this to the 1.21.0 milestone Jun 28, 2022
@cortinico cortinico added the notable changes Marker for notable changes in the changelog label Jun 28, 2022
@cortinico
Copy link
Member

I think that something like this could do the trick on the changelog:
We updated the baseline creation. Now the kdoc is not part of the function signature. This could create that already suppressed issues in your baseline reappear. To fix that just recreate your baseline.

I think KDoc comments were trimmed before that too.

element.docComment?.let {
    methodStart = it.endOffset - it.startOffset
}

Can we verify this. If we require users to regenerate baselines because of trimming of KDocs, that should be reported clearly in the changelog 👍

@VitalyVPinchuk
Copy link
Contributor Author

@cortinico No, I was wrong!
Previous revision doesn't pass current tests because of leaving comments in the signature!

org.opentest4j.AssertionFailedError: 
expected: "Test.kt$fun data()"
 but was: "Test.kt$/* comments */ fun data()"

Consider the issue #4767

@cortinico
Copy link
Member

Previous revision doesn't pass current tests because of leaving comments in the signature!

Still this doesn't answer my original question: do we need to ask users to regenerate baselines?

@VitalyVPinchuk
Copy link
Contributor Author

Still this doesn't answer my original question: do we need to ask users to regenerate baselines?

Yes, we do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
migration Marker to add a migration step in the changelog notable changes Marker for notable changes in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants