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

Go comments not being cleaned up correctly #376

Closed
conormurray95 opened this issue Feb 16, 2023 · 4 comments · Fixed by #392
Closed

Go comments not being cleaned up correctly #376

conormurray95 opened this issue Feb 16, 2023 · 4 comments · Fixed by #392
Labels
bug Something isn't working

Comments

@conormurray95
Copy link

conormurray95 commented Feb 16, 2023

Issue
When running the tool with cleanup_comments = true against a go codebase I'm seeing some inconsistent behaviour around comment cleanup.

Steps to reproduce
I've added test cases demonstrating this issue in this pr #375

Observations:

  • Multiline comments are cleaned up
  • Single line comments are not cleaned up
  • Single line comments are cleaned up provided the next node above is also a comment

The test cases provided in the pr are for cleaning up if blocks however I've noticed the same behaviour when cleaning up constants in my own codebase e.g.

Before

const (
    // comment
    myFlag = "flag"

   // stale comment
   myStaleFlag = "stale flag"
)

After

const (
    // comment
    myFlag = "flag"

   // stale comment
)

Expected

const (
    // comment
    myFlag = "flag"
)

The constants example above exhibits the same behaviour around only cleaning up comments if they're (a) multiline or (b) have another comment node directly above them.

Any context on if this is a known issue in other languages or seems go specific etc would be great. It's the one piece left stopping autogenerated pr's we have being able to be directly merged.

@ketkarameya
Copy link
Collaborator

Hey! Thanks a lot for reporting this.
the go cleanup mechanism we have is pretty much experimental :| We have not battle tested it yet (by running it on our internal codebase) But thanks for doing this :) I will look into it. Like our java/kotlin cleanup is kinda robust.
cc @dvmarcilio

@dvmarcilio
Copy link
Collaborator

thanks for pinging me, @ketkarameya.

Thanks for the detailed report, @conormurray95!
It nicely outlines the comments-cleanup inconsistencies I saw when experimenting with the Go rules.

I'll try to look closer into this in the upcoming days. Hopefully, I can pinpoint a cause.

@ketkarameya ketkarameya added the bug Something isn't working label Feb 21, 2023
@ketkarameya
Copy link
Collaborator

@conormurray95 can you check this branch go_comment_bug #392 on your go codebase, and let me know if it works as expected?

@ketkarameya ketkarameya linked a pull request Feb 23, 2023 that will close this issue
@conormurray95
Copy link
Author

Hey @ketkarameya apologies I just got a chance to look back at this, that pr looks good and solves the cases I'd spotted. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants