Add copyright check

[KyleFromNVIDIA]

Fixes: #2

[bdice]

(Initial pass -- these comments may be outdated by the latest changes.)

[bdice]

At this point, the code looks good and I just need to spend time testing before I approve.

[bdice]

This failed for me when I tested this hook on cudf. I got a UnicodeDecodeError because I forgot to exclude binary files (*.parquet, *.orc, etc.). Should we patch to ignore binary files, and possibly warn in those cases?

diff --git a/src/rapids_pre_commit_hooks/lint.py b/src/rapids_pre_commit_hooks/lint.py
index edb1f93..4f8ea9c 100644
--- a/src/rapids_pre_commit_hooks/lint.py
+++ b/src/rapids_pre_commit_hooks/lint.py
@@ -210,7 +210,10 @@ class ExecutionContext(contextlib.AbstractContextManager):
 
         for file in self.args.files:
             with open(file) as f:
-                content = f.read()
+                try:
+                    content = f.read()
+                except UnicodeDecodeError:
+                    pass
 
             linter = Linter(file, content)
             for check in self.checks:

[KyleFromNVIDIA]

Should we patch to ignore binary files, and possibly warn in those cases?

I'll open a separate PR for that.

Edit: #10

[bdice]

I tried testing this hook on cuspatial. That repo doesn't have any automated enforcement of copyrights today, so many of its files are outdated. I tried editing some files, and the copyright script appeared to work as intended.

However, I am not sure what options I can provide to get this repo "up to date." There are a lot of files that were modified in past PRs in 2023 and 2024 but those changes aren't reflected in the copyright dates because this repo had no copyright year header enforcement. It seems like this hook might provide eventual consistency after every file is modified, but I want to batch-process the existing files and get them caught up so that the copyrights on branch-24.04 reflect the dates that the files were last modified in the git history. Is there a way to do this? Is that an option we can provide?

Testing PR here: rapidsai/cuspatial#1328

[harrism]

Is it really essential to correct cuSpatial? Seems to me "eventual consistency" is good enough.

[bdice]

Is it really essential to correct cuSpatial? Seems to me "eventual consistency" is good enough.

I would accept eventual consistency if a bulk update is impossible, but I want to make sure I'm understanding the scope and capabilities of this tool correctly. I think it would be reasonable for this kind of bulk update to be in scope, too, and it shouldn't be very complex if my understanding is correct.

[KyleFromNVIDIA]

I want to batch-process the existing files and get them caught up so that the copyrights on branch-24.04 reflect the dates that the files were last modified in the git history. Is there a way to do this? Is that an option we can provide?

Right now the script does "update all files with the current year that have been modified since the merge-base with the target branch." The batch use case is "update all files with the year they were last modified at any point in their Git history."

The script does not do this in its current state. It is definitely possible, and I'd be happy to do it, but it will require a fair amount of rework. We will have to trace the history of each file, including through copies and renames. Let's get this first iteration merged first, and then we can do that as a follow-up.

Edit: I added this feature to this PR. You can try it with the --batch argument. We may be able to merge these two modes later on, but for now I'm keeping them separate.

[KyleFromNVIDIA]

I just ran --batch mode on all of cudf (2798 files, 37945 commits.) It completed in a reasonable-ish amount of time (18 minutes (98,000 file-commits per second)), and AFAICT it didn't make any mistakes.

[KyleFromNVIDIA]

cuspatial (570 files, 2006 commits) completed in 13 seconds (88,000 file-commits per second.)

[bdice]

A few final comments, but overall I think this is really close. I approve. Feel free to iron out any details and ask for more feedback if you want it, but feel free to merge when you're ready and we can start to migrate RAPIDS to using this hook.