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

Use standard function to check if a file is generated #1114

Merged
merged 4 commits into from
Mar 8, 2024
Merged

Use standard function to check if a file is generated #1114

merged 4 commits into from
Mar 8, 2024

Conversation

nobishino
Copy link
Contributor

@nobishino nobishino commented Mar 7, 2024

As of Go1.21, we can utilize standard package function ast.IsGenerated to check if a file is generated or not. This PR replace isGeneratedFile with that new standard function.

Second commit updates test source.
Standard ast.IsGenerated returns false for original test sources, because it does not comply with
https://pkg.go.dev/cmd/go#hdr-Generate_Go_files_by_processing_source

This line must appear before the first non-comment, non-blank text in the file.

Original test source does not satisfy this condition, so original isGeneratedFile function returns true when it should return false.

I would assume we should update the test cases, but this change can be breaking for some users. I wonder if we should do that.

As of Go1.21, we can use https://pkg.go.dev/go/ast#IsGenerated to check if a file is generated.
Probably we want to use this instead of own implementation.
https://pkg.go.dev/cmd/go#hdr-Generate_Go_files_by_processing_source says:

> This line must appear before the first non-comment, non-blank text in the file.

Original test cases fail with the previous commit because test source does not comply with this spec.
So, probably we should update test case to comply with the spec.
(This is a breaking change, though)
@nobishino
Copy link
Contributor Author

I noticed that Ci is failing, I will look into it later.

Copy link
Member

@ccojocar ccojocar left a comment

Choose a reason for hiding this comment

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

Thanks for this update. Please could you fix the lint warning?

@nobishino
Copy link
Contributor Author

@ccojocar I've tried to fix it, but I don't understand the cause of the lint error:

  Error: File is not `gci`-ed with --skip-generated -s standard -s default -s prefix(github.com/securego) (gci)
  Error: File is not `gci`-ed with --skip-generated -s standard -s default -s prefix(github.com/securego) (gci)
  
  Error: issues found

I would appreciate any advice.

@ccojocar
Copy link
Member

ccojocar commented Mar 8, 2024

I think you need to run the gci tool to format the two files. Something like this:

 gci write --skip-generated -s standard -s default -s prefix(github.com/securego)  analyzer.go

You can check if the warnings are fixed by running locally the golangci-lint tool.

Signed-off-by: Cosmin Cojocar <gcojocar@adobe.com>
@ccojocar
Copy link
Member

ccojocar commented Mar 8, 2024

@nobishino I did the formatting. Let's see if now passes the lint.

@ccojocar ccojocar merged commit 48aa72e into securego:master Mar 8, 2024
6 checks passed
@nobishino
Copy link
Contributor Author

@ccojocar Thanks!

@nobishino nobishino deleted the refactor-use-standard-function-to-check-generated-file branch March 8, 2024 11:22
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.

None yet

2 participants