Add support for .lr and .lr.go files#85
Conversation
|
|
CalebAlbers
left a comment
CalebAlbers
left a comment
There was a problem hiding this comment.
Hey, @czunker 👋
Thanks a ton for the contribution! I'm glad to see copywrite gaining use in the community.
I left a few small notes on this I'd like addressed, but happy to merge after that.
Cheers!
| case ".js", ".mjs", ".cjs", ".jsx", ".tsx", ".css", ".scss", ".sass", ".ts": | ||
| lic, err = executeTemplate(tmpl, data, "/**", " * ", " */") | ||
| case ".cc", ".cpp", ".cs", ".go", ".hh", ".hpp", ".m", ".mm", ".proto", ".rs", ".swift", ".dart", ".groovy", ".v", ".sv": | ||
| case ".cc", ".cpp", ".cs", ".go", ".hh", ".hpp", ".m", ".mm", ".proto", ".rs", ".swift", ".dart", ".groovy", ".v", ".sv", ".lr.go", ".lr": |
There was a problem hiding this comment.
I believe the .lr.go case should be taken care of automatically given it ends in .go. The module uses Go's filepath.Ext() function, which should look only at the last suffix according to the docs:
Ext returns the file name extension used by path. The extension is the suffix beginning at the final dot in the final element of path; it is empty if there is no dot.
| case ".cc", ".cpp", ".cs", ".go", ".hh", ".hpp", ".m", ".mm", ".proto", ".rs", ".swift", ".dart", ".groovy", ".v", ".sv", ".lr.go", ".lr": | |
| case ".cc", ".cpp", ".cs", ".go", ".hh", ".hpp", ".m", ".mm", ".proto", ".rs", ".swift", ".dart", ".groovy", ".v", ".sv", ".lr": |
There was a problem hiding this comment.
Yes, you are correct. It was skipped because .lr.go are outgenerated files. I found that later in the code.
| return err | ||
| } | ||
| if lic == nil { // Unknown fileExtension | ||
| logger.Printf("[DEBUG] skipping because of unknown extension: %s", f.path) |
There was a problem hiding this comment.
I like this idea, but the current logging implementation needs some rework, so this will currently print out a large number of debug noise during regular noise.
Do you mind if we remove it for this PR so we can ship the changes and log it as a GitHub Issue instead?
| case ".js", ".mjs", ".cjs", ".jsx", ".tsx", ".css", ".scss", ".sass", ".ts": | ||
| lic, err = executeTemplate(tmpl, data, "/**", " * ", " */") | ||
| case ".cc", ".cpp", ".cs", ".go", ".hh", ".hpp", ".m", ".mm", ".proto", ".rs", ".swift", ".dart", ".groovy", ".v", ".sv": | ||
| case ".cc", ".cpp", ".cs", ".go", ".hh", ".hpp", ".m", ".mm", ".proto", ".rs", ".swift", ".dart", ".groovy", ".v", ".sv", ".lr.go", ".lr": |
There was a problem hiding this comment.
Do you mind adding an entry into our test suite, as well? The specific lines are https://github.com/hashicorp/copywrite/blob/main/addlicense/main_test.go#L316-L317
Signed-off-by: Christian Zunker <christian.zunker@gmail.com>
czunker
force-pushed
the
christian/support_lr
branch
from
21cc6c8 to
c370d35
Compare
|
Thanks for your feedback @CalebAlbers. I addressed your comments. Please have another look. |
3 participants
🛠️ Description
Add new file extensions to also support license headers for
.lrand.lr.gofiles.🔗 External Links
We use it in this repo: https://github.com/mondoohq/cnquery
👍 Definition of Done
🤔 Can be merged upon approval?
✅