Copyright notices: script and pre-commit hook (#13)#19
Conversation
blms
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #19 +/- ##
=======================================
Coverage 85.21% 85.21%
=======================================
Files 15 15
Lines 2165 2165
=======================================
Hits 1845 1845
Misses 320 320 🚀 New features to boost your workflow:
|
It feels weird to be inconsistent, but I also don't love the long-form notice. I think let's leave it. Would it be possible to add
I think let's not bother - it's a utility script, not part of the library itself. Let's exempt github workflows from the copyright - those are mostly borrowed from other projects or examples anyway. |
rlskoeser
left a comment
rlskoeser
left a comment
There was a problem hiding this comment.
I pulled this branch and installed the pre-commit hooks, and then ran with pre-commit run --all.
It reports that check-copyright fails because of the files that don't have a copyright notice.
Will this cause any problems when committing changes to those files?
That's my only concern, I think other than making sure that's not a problem this is good to merge.
|
@rlskoeser Thanks for testing locally! Good point. Since our script can't check for those files' comment syntax, it will never pass for them. So we should either exempt them from the script, or add their comment syntax to the checkable types so that it will actually pass once someone adds a copyright notice to them. |
@blms for now can you exempt them by using the file pattern option in the pre-commit hook, so we only check the files where failure means it should not be committed? |
blms
force-pushed
the
feature/13-copyright-script
branch
from
8bd6dba to
ebc128b
Compare
rlskoeser
left a comment
rlskoeser
left a comment
There was a problem hiding this comment.
Ran the updated pre-commit check and no failures.
Also ran the script directly a few times to see - seems fine for what we need.
2 participants
Associated Issue(s): #13
Changes in this PR
--checkargument to report on files missing notices (in this mode, will not modify files)--checkon all new/changed files, which will fail if notices are missingNotes
I used the SPDX shorthand for adding notices to files that don't have one, does that make sense to you or should we be more consistent and use the old long-form notice?
Should I add unit tests for the script?
For now the script only supports
#style comments in python, yaml, and toml; running against the whole repo with--checkshows these additional files missing notices. Let me know if I should address or if it's ok for now.Missing copyright report