Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork1.5k
feat: add golicenser linter#5751
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Hey, thank you for opening your first Pull Request ! |
CLAassistant commentedApr 19, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
|
ldez commentedApr 19, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
In order for a pull request adding a linter to be reviewed, the linter and the PR must follow some requirements.
Pull Request Description
Linter
The Linter Tests Inside Golangci-lint
|
This linter is obviously a duplicate of I would have preferred an enhancement of Currently, I note a "missing" option compared to |
ldez commentedApr 19, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Also, there is a major and problematic difference with If a fix is applied, the following comments are converted to /*Copyright (c) 2025 golangci-lint <test@example.com>.This file is a part of golangci-lint.*/ /*Copyright (c) 2025 golangci-lint <test@example.com>.This file is a part of golangci-lint.*/ /* Copyright (c) 2025 golangci-lint <test@example.com>.This file is a part of golangci-lint. */ Another problem: the whitespaces inside the template are not respected if they are at the beginning of a sentence. linters:settings:golicenser:header:template:|- Copyright (c) {{.year}} {{.author}} <{{.email}}>. This file is a part of {{.projectname}}. (There are extra spaces before each sentence.) |
ldez commentedApr 19, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
The linter has the same problem as /* * header text * header text */ The linter fixes remove build directives: //go:build unix && !foopackage testdata The linter only checks headers that match the |
joshuasing commentedApr 19, 2025 • edited by ldez
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by ldez
Uh oh!
There was an error while loading.Please reload this page.
Agreed, sorry I meant to point that out in the PR but forgot. I had originally used goheader for many of my projects, and ended up writing golicenser as a replacement due to said limitations and design problems. Ideally, golicenser is meant to replace goheader in many ways.
Hmm, this is implemented in the
golicenser supports setting a "comment style", where either "line" or "block" can be used - the idea of this was to help keep all license headers as consistent as possible. Perhaps a "preserve" comment style, which behaves in this way, could be a better default?
That's interesting, I will look into this. I am sure I have tested this without the yaml config and it worked before, so I wonder if it's some parsing difference 🤔
Ah, comment style "block" will render as: /*comment*/ to match how Kubernetes' license headers are formatted. The "preserve" comment style suggested above could fix this as well, otherwise a separate comment style for rendering the star on each line would work 🤔
That is definitely not intentional, will fix. Thank you.
The linter should check headers against two regexps: the copyright header regexp, then the license header matcher:
Is this the observed behaviour, and does this behaviour make sense? |
I think that
This is unintuitive, so too complex IMHO. |
ldez commentedApr 19, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Additional context: I was discussing with the And I notified the author in DM about this PR. |
joshuasing commentedApr 19, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Okay, will do.
Hm okay. The reason for this was primarily so that projects that contain files with different licenses could ignore such files easily, however these could also be excluded individually, I suppose. I will rewrite this.
Great. My main goal is to end up with a license header linter which is easy to use and works for any project, so if there's anything I can do to help achieve that, please let me know. Thanks for all of the feedback so far! |
joshuasing commentedApr 19, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Added I have reproduced the build directive issue locally: The |
ldez commentedApr 19, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
You must remove this:https://github.com/joshuasing/golicenser/blob/e9a45b2af8f63428fd666fa8c6425f0f8b82ac8d/analysis.go#L51-L53
The go tooling ignores directories starting with
|
joshuasing commentedApr 20, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Thanks for pointing this out. I have removed it here:joshuasing/golicenser#10 For this note: Setting MaxConcurrent to
I have createdjoshuasing/golicenser#12 to add a "starred-block" comment style, which renders as:
The pull request also improves comment parsing to properly handle such comments, as well as changing the header detection to ignore directive comments and make sure they are never replaced. I will release the above changes as Note: I have also moved the position used in diagnostics to Any feedback is highly appreciated, thank you. |
For me, the current configuration is unintuitive and too complex. IMHO, the following options should be removed:
Also, It could be great to not use the default template markers I think your goal is to handle multiple headers (not currently implemented). I suggest the following configurations:
golicenser:comment-style:"line"template:|- Copyright (c) [[.year]] [[.author]]. All rights reserved. Use of this source code is governed by a BSD-style license that can be found in the LICENSE file.# template-path: /path/to/my/template.tmplyear-mode:"git-range"variables:author:match:"(My Name|Your Name)"replacement:"My Name"email:match:"(.+)@mycompany\\.com"replacement:"foo@example.com"project-name:value:"My project"
golicenser3:default-header:bsd# if there is only one header definition, use it as default.headers:# map[string]Optionsbsd:matcher:'BSD-style'# Matches on existing headers. (Empty -> all files)comment-style:"line"template:|- Copyright (c) [[.year]] [[.author]]. All rights reserved. Use of this source code is governed by a BSD-style license that can be found in the LICENSE file.# template-path: /path/to/my/template.tmplyear-mode:"git-range"variables:author:match:"(My Name|Your Name)"replacement:"My Name"email:match:"(.+)@mycompany\\.com"replacement:"foo@example.com"project-name:value:"My project"apache:matcher:'Apache License'# Matches on existing headers. (Empty -> all files)comment-style:"line"template:|- Copyright [[.year]] [[.author]]Licensed under the Apache License, Version 2.0 (the "License");you may not use this file except in compliance with the License.# template-path: /path/to/my/template.tmplyear-mode:"git-range"variables:author:match:"(My Name|Your Name)"replacement:"My Name"email:match:"(.+)@mycompany\\.com"replacement:"foo@example.com"project-name:value:"My project" OR golicenser3:default-header:bsd# if there is only one header definition, use it as default.headers:# map[string]Optionsbsd:matcher:my/package/one# Match on the package path. (Empty -> match all except other rules)comment-style:"line"template:|- Copyright (c) [[.year]] [[.author]]. All rights reserved. Use of this source code is governed by a BSD-style license that can be found in the LICENSE file.# template-path: /path/to/my/template.tmplyear-mode:"git-range"variables:author:match:"(My Name|Your Name)"replacement:"My Name"email:match:"(.+)@mycompany\\.com"replacement:"foo@example.com"project-name:value:"My project"apache:matcher:my/package/two# Match on the package path. (Empty -> match all except other rules)comment-style:"line"template:|- Copyright [[.year]] [[.author]]Licensed under the Apache License, Version 2.0 (the "License");you may not use this file except in compliance with the License.# template-path: /path/to/my/template.tmplyear-mode:"git-range"variables:author:match:"(My Name|Your Name)"replacement:"My Name"email:match:"(.+)@mycompany\\.com"replacement:"foo@example.com"project-name:value:"My project" |
joshuasing commentedApr 21, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Thanks for the feedback! I agree it is currently too complex, and many improvements can be made. For these options:
I will rewrite the matching system and implement these changes during the week.
As in change the
golicenser was primarily designed to lint the project's own license headers, since most projects only have one license. I could implement support for multiple headers, which I suppose allows projects to enforce different headers/licenses in different areas of the codebase. 🤔 How useful would this be and how often would this likely be used? Thanks again. Quick update (May 6th): I have been working on this! I am about to go on an overseas trip and will be back early next month, when I will continue the work on golicenser and this PR. If possible, please keep this open for now. Thanks! |
Uh oh!
There was an error while loading.Please reload this page.
Add golicenser (https://github.com/joshuasing/golicenser) linter.
golicenser aims to be a fast, easy-to-use and highly customisable linter for managing license headers.
I have done my best to make sure this pull request meets the requirements for new linters, however if I have missed anything, please let me know.
I am the author and maintainer of golicenser, and I would love to see it included in golangci-lint. I am more than happy to make any necessary changes to this pull request or golicenser itself, and any feedback would be greatly appreciated. Thank you.