Skip to content

Conversation

denis-tingaikin
Copy link
Owner

@denis-tingaikin denis-tingaikin commented May 10, 2025

    1. #35

    2. #43

Signed-off-by: Denis Tingaikin <[email protected]>
Signed-off-by: Denis Tingaikin <[email protected]>
Signed-off-by: Denis Tingaikin <[email protected]>
Signed-off-by: Denis Tingaikin <[email protected]>
Signed-off-by: Denis Tingaikin <[email protected]>
Signed-off-by: Denis Tingaikin <[email protected]>
Signed-off-by: Denis Tingaikin <[email protected]>
Signed-off-by: Denis Tingaikin <[email protected]>
Signed-off-by: Denis Tingaikin <[email protected]>
Signed-off-by: Denis Tingaikin <[email protected]>
@denis-tingaikin denis-tingaikin mentioned this pull request May 10, 2025
Signed-off-by: Denis Tingaikin <[email protected]>
@denis-tingaikin
Copy link
Owner Author

Summons @ldez , @firelizzard18

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR prepares the v1.0.0 release by updating copyright years, refactoring the configuration to use a new "Config" type with "vars" instead of "values", and removing legacy code files.

  • Renames Configuration to Config and replaces "values" with "vars" within configuration files and related tests.
  • Removes legacy source files (reader.go, location.go, issue.go) and updates the main command to use flag-based configuration and parallel processing.
  • Upgrades the Go version in go.mod and adds new dependency modules while updating documentation and usage instructions.

Reviewed Changes

Copilot reviewed 31 out of 31 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
testdata/golangci-linter/sample.yml Updated template and values usage with dot notation.
testdata/fix/fix.yml Replaces "values" with "vars" and renames keys for consistency.
testdata/constvalue/constvalue.yml Changes from "const" under "values" to using "vars".
testdata/cgo/* Minor updates for dot notation in templates.
reader.go, location.go, issue.go Legacy files removed.
option.go Removed lowercasing of keys in WithValues, preserving case.
config.go Renamed type and adjusted error handling in GetTemplate and GetValues.
cmd/go-header/main.go Refactored main function and flag handling.
analyzer_test.go Test adjustments to match new config format and tempDir usage.
analyzer.go & analysis.go Major refactor of analysis and fix generation with parallel processing.
README.md Updated installation, usage, and feature table with minor text changes.
.go-header.yml Updated configuration to match new vars format and key naming.
.github/workflows/ci.yml Adjusted Go version and file path usage in CI.
Comments suppressed due to low confidence (3)

config.go:77

  • Returning an empty string instead of an error when TemplatePath is missing may lead to silent failures. Consider providing a clear error message instead.
if c.TemplatePath == "" {

.go-header.yml:2

  • [nitpick] There is a potential inconsistency between the variable naming in configuration and documentation (using an underscore here versus a hyphen elsewhere). Ensure that key names like MOD_YEAR_RANGE are documented and used consistently.
copyright_holder: "{{ .MOD_YEAR_RANGE}} Denis Tingaikin"

option.go:33

  • [nitpick] The change from lowercasing configuration keys to preserving their original casing may affect users if configuration files use lower-case keys. Verify that this change is intentional and documented.
a.values[k] = v

@denis-tingaikin
Copy link
Owner Author

Golangci-linter update: golangci/golangci-lint#5791

@denis-tingaikin denis-tingaikin marked this pull request as ready for review May 10, 2025 18:28
@ldez
Copy link
Collaborator

ldez commented May 10, 2025

It could be great to not use the default template markers {{ .XXX }}.

@denis-tingaikin
Copy link
Owner Author

FYI: Golangci tests result on the latest commit https://github.com/golangci/golangci-lint/actions/runs/14947962804/job/41993802062

@denis-tingaikin
Copy link
Owner Author

denis-tingaikin commented May 10, 2025

It could be great to not use the default template markers {{ .XXX }}.

Do you mean the backward compatibility with {{ XXX }}?

@ldez
Copy link
Collaborator

ldez commented May 10, 2025

Do you mean the backward compatibility with {{ XXX }}?

No, I mean not to use go template default markers {{ }}.

But the breaking change is also a problem.

@denis-tingaikin
Copy link
Owner Author

But the breaking change is also a problem.

I'm just thinking about adding a simple migration mechanism on these changes and rollback changes here in configuration golangci/golangci-lint#5791

Does it sound acceptable to resolve the comment?

@denis-tingaikin
Copy link
Owner Author

No, I mean not to use go template default markers {{ }}.

Currently, go-header is integrated with text/template, so template markers are acceptable and go template functions can be used by folks in headers. 😀

Please add more details if I missed something.

@ldez
Copy link
Collaborator

ldez commented May 10, 2025

If the configuration of linter uses the default Go template markers, it is a problem if I want to use Go templates on the global configuration.

I asked the same thing to golicenser.

@denis-tingaikin
Copy link
Owner Author

Is marker {{ XXX }} acceptable, or should we do a break change with the previous go-header version and use another marker?

@ldez
Copy link
Collaborator

ldez commented May 10, 2025

I would prefer a compatibility layer:

  • the "default" markers will be [[/]] (for example)
  • the old goheader template {{ XXX }} can be converted to [[ .XXX ]].

Signed-off-by: Denis Tingaikin <[email protected]>
@denis-tingaikin
Copy link
Owner Author

@ldez Thanks, done.

@denis-tingaikin denis-tingaikin requested a review from ldez May 11, 2025 19:54
@ldez
Copy link
Collaborator

ldez commented May 11, 2025

This is not really what I expected: you are still using {{/}}.

I was expecting something like that:

template.New("header").Delims("[[","]]")

Maybe the delimiters can be an option (we will not expose them inside the golangci-lint configuration).

End, Start token.Pos
}

func (a *Analyzer) skipCodeGen(file *ast.File) ([]*ast.CommentGroup, []*ast.Comment) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

ast.IsGenerated(file)

Copy link
Owner Author

Choose a reason for hiding this comment

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

the function skipCodeGen is still needed to pass test with cgo from the golangci-linter repo. ast.IsGenerated(file) applied in another place, thanks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that cgo should not be supported by this linter because the header of a cgo file is special, which will lead to false positives.

@denis-tingaikin
Copy link
Owner Author

denis-tingaikin commented May 11, 2025

This is not really what I expected: you are still using {{/}}.

OK, I've used delims in text/template. Also, added tests:

  1. with migration configs (see test sample 'testdata/oldconfig/**')
  2. new configs where folks may use '[[' and ']]' in header templates. (see test sample 'testdata/constvalue2/**')

Also, checked that tests still passing on this branch golangci/golangci-lint#5791

Do we need to fix anything else before merging this?

@ldez
Copy link
Collaborator

ldez commented May 11, 2025

Do we need to fix anything else before merging this?

It depends on what you expect of the PR.
If you plan to create a release only with this PR, yes.
If not, you can merge, and I will open a PR to apply some changes.

@denis-tingaikin
Copy link
Owner Author

The current goal is to fix issues from folks and be able to update go-header in golangci-lint.

OK, I think it's a good starting point to continue improving the go-header.

My next plan is:

  1. Poke with release candidates for some time and do changes if I find something.
  2. If you or some of the folks will open PR, then we'll include it for the next release.
  3. If all will be good with №1 and №2, then I'll create a release and reopen Bump: update go header to v1.0.0-rc.5 golangci/golangci-lint#5791

@ldez Many thanks for the assistance, and feel free to contribute changes ;)

@denis-tingaikin denis-tingaikin merged commit 2b3a0be into main May 11, 2025
2 checks passed
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.

2 participants