-
Notifications
You must be signed in to change notification settings - Fork 10
Release v0.6.0 #45
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
Release v0.6.0 #45
Conversation
@ldez, @firelizzard18 would be appreciated if you could review these changes. UPD: Going to merge this next week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
I suggest merging PR #47 before and rewriting the tests of this PR based on the other one. |
} | ||
|
||
file := target.File | ||
var header string | ||
var offset = Location{ | ||
Position: 1, | ||
} | ||
|
||
if isNewLinewRequired(file.Comments) { | ||
return NewIssueWithLocation( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally this would include a fix so golangci-lint can automatically apply it. IIUC the Line value will point to the line after the comment as being the error location, so I think Fix{Actual: []string{}, Expected: []string{""}},
would be sufficient. Unfortunately my attempt to test it failed - isNewLineRequired
is returning false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ldez Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
golangci-lint can already apply fixes.
I think the current implementation based on io.reader
is hard to maintain and has some limitations.
analyzer.go
Outdated
func isNewLinewRequired(group []*ast.CommentGroup) bool { | ||
if len(group[0].List) > 1 { | ||
for _, item := range group[0].List { | ||
if strings.HasPrefix(item.Text, "/*") { | ||
return true | ||
} | ||
} | ||
} | ||
|
||
if len(group) < 2 { | ||
return false | ||
} | ||
return group[0].End() >= group[1].Pos() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand how this works. I tried to test adding a fix but in both cases below isNewLineRequired
returns false.
/* mycompany.com
SPDX-License-Identifier: Foo */
package foo
// mycompany.com
// SPDX-License-Identifier: Foo
package foo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would also be nice to detect this:
// mycompany.com
// SPDX-License-Identifier: Foo
// Package foo
package foo
@firelizzard18 FYI, I made some suggestions about the linter (not this PR) to @denis-tingaikin in DM, and one of them is to rewrite the linter to use IMHO, the current implementation based on |
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]>
dbfdac9
to
123ab1f
Compare
Signed-off-by: Denis Tingaikin <[email protected]>
4d85685
to
ebb64bf
Compare
Signed-off-by: Denis Tingaikin <[email protected]>
a8f7ebd
to
5d89dc8
Compare
Signed-off-by: Denis Tingaikin <[email protected]>
5d89dc8
to
0c76dad
Compare
There was a problem hiding this 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 fixes header handling issues by ensuring there's an explicit check for an empty newline immediately after the header and correctly processing unicode headers. It also refactors configuration types (renaming from Configuration to Config), updates test cases accordingly, and modernizes string handling in reader.go.
- Introduces a newline check after the header with a clear message.
- Renames Configuration to Config and adjusts associated helper functions.
- Updates tests and documentation to support unicode headers and new configuration format.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
testdata/starcomment/starcomment.yml | Updates header template and variable naming for star comment |
testdata/starcomment/starcomment.go | Adjusts header processing for star block comments |
testdata/nestedvalues/nestedvalues.yml | Replaces deprecated "values" with "vars" |
testdata/fix/fix.yml | Corrects header placeholder from MY COMPANY to .MY_COMPANY |
testdata/constvalue/constvalue.yml | Changes header variable interpolation from YEAR to .YEAR |
reader.go | Converts string source to []rune and refines header parsing |
go.mod | Bumps Go version from 1.21 to 1.24 |
config.go | Renames Configuration to Config and updates field names |
cmd/go-header/main.go | Refactors main to use the new Config type |
analyzer_test.go | Updates test cases reflecting the new header and config format |
analyzer.go | Implements newline check for header and additional helper funcs |
README.md | Updates configuration instructions to align with new config vars |
.github/workflows/ci.yml | Excludes testdata folders from CI runs |
Comments suppressed due to low confidence (1)
reader.go:103
- The function 'min' is used here but is not defined anywhere. Consider defining a helper function (e.g., func min(a, b int) int { ... }) or use an inline comparison to determine the minimum.
minVal := min(len(r.source), r.position)
Signed-off-by: Denis Tingaikin <[email protected]>
06d06b4
to
8186b8e
Compare
Signed-off-by: Denis Tingaikin <[email protected]>
Signed-off-by: Denis Tingaikin <[email protected]>
There was a problem hiding this 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 pull request implements the v1.0.0 release addressing several issues by updating version references, refactoring configuration and header analysis logic, and refreshing tests, documentation, and CI workflow settings.
- Update version strings and copyright dates
- Refactor configuration handling by renaming Configuration to Config and merging vars into value processing
- Improve header analysis in Analyzer with enhanced newline and star block handling, plus integrate go/analysis
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
version/version.go | Update version string to "v1.0.0" |
testdata/starcomment/* | Update YAML and Go files to use new header template syntax |
testdata/nestedvalues/nestedvalues.yml | Switch from deprecated “values” to “vars” format |
testdata/fix/fix.yml | Replace raw key formatting with dot notation (.MY_COMPANY) |
testdata/constvalue/constvalue.yml | Update template syntax for constant values |
reader.go | Change source type from string to []rune and rename variable |
go.mod | Update Go version to 1.24 and add required tools |
config.go | Rename Configuration to Config; update GetValues and other methods |
cmd/go-header/main.go | Enhance command line flag handling and switch to Analyzer |
analyzer_test.go | Modify tests to use t.TempDir() and adopt the new config type |
analyzer.go | Refactor Analyzer to use getPerTargetValues and add newline/star block handling |
analysis.go | Add new go/analysis Analyzer integration |
README.md | Revise documentation and configuration examples |
.go-header.yml | Update configuration format from “values” to “vars” |
.github/workflows/ci.yml | Update CI workflow to use Go 1.24 and adjust file targeting |
Signed-off-by: Denis Tingaikin <[email protected]>
eebdd7a
to
c7b2750
Compare
@ldez , @firelizzard18 I just decided to rework everything with modern go tools for fun, please have a look at #49 |
Motivation
Fixes next issues:
1. #35
2. #43
3. #46
4. #44
5. #41