Skip to content
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

Fix make copyright #522

Merged
merged 1 commit into from
Jan 19, 2023
Merged

Fix make copyright #522

merged 1 commit into from
Jan 19, 2023

Conversation

Fleshgrinder
Copy link
Contributor

The make copyright goal always lists all Python files that have a copyright notice, and since we have more than one file with a copyright notice it always exits with success. This does not seem right, given that there is a GHA job that is supposed to check that all files (except __init__.py) have a copyright notice. This changes the code used in the goal to a single git invocation that does exactly that. The ! in front inverts the exit code. Hence, if a Python file other than __init__.py is found without a copyright notice it exists with a non-zero exit code and lists the paths that miss the copyright notice relative to the repository root.

@Fleshgrinder Fleshgrinder requested review from a team as code owners January 19, 2023 12:17
@Fleshgrinder Fleshgrinder self-assigned this Jan 19, 2023
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Jan 19, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 0d0ede8
Status: ✅  Deploy successful!
Preview URL: https://b6fa462e.karapace.pages.dev
Branch Preview URL: https://copyright.karapace.pages.dev

View logs

@Fleshgrinder Fleshgrinder force-pushed the copyright branch 2 times, most recently from 4ebcaa1 to 61dc91f Compare January 19, 2023 12:48
@Fleshgrinder
Copy link
Contributor Author

Updated all Python files with the copyright notice, also updated the year since I had to touch the all anyway. Is there a reason why we have the copyright notice everywhere? It is not required, the LICENSE file in the root is sufficient. Removing the existing license headers would be simpler than this patch. 😉

.PHONY: copyright
copyright:
grep -EL "Copyright \(c\) 20.* Aiven" $(shell git ls-files "*.py" | grep -v __init__.py)
! git grep --untracked -ELm1 'Copyright \(c\) 20[0-9]{2} Aiven' -- '*.py' ':!*__init__.py'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only code change in this patch. Notice that the --untracked is important for the goal to make sense locally.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM: git grep finding a *.py file (excluding __init__.py) without the regex causes the target to fail.

The `make copyright` goal always lists all Python files that have a copyright
notice, and since we have more than one file with a copyright notice it always
exits with success. This does not seem right, given that there is a GHA job that
is supposed to check that all files (except `__init__.py`) have a copyright
notice. This changes the code used in the goal to a single git invocation that
does exactly that. The `!` in front inverts the exit code. Hence, if a Python
file other than `__init__.py` is found without a copyright notice it exists with
a non-zero exit code and lists the paths that miss the copyright notice relative
to the repository root.
@jlprat
Copy link
Contributor

jlprat commented Jan 19, 2023

While no strictly needed, some best practices still recommend to add the copyright notices on the files. I guess this makes more sense in projects whose files can be easily copied over to other projects. This way its easier for the ones copying to apply the proper attribution.

.PHONY: copyright
copyright:
grep -EL "Copyright \(c\) 20.* Aiven" $(shell git ls-files "*.py" | grep -v __init__.py)
! git grep --untracked -ELm1 'Copyright \(c\) 20[0-9]{2} Aiven' -- '*.py' ':!*__init__.py'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM: git grep finding a *.py file (excluding __init__.py) without the regex causes the target to fail.

@RyanSkraba RyanSkraba merged commit ee88849 into main Jan 19, 2023
@RyanSkraba RyanSkraba deleted the copyright branch January 19, 2023 17:22
@RyanSkraba
Copy link
Contributor

Thanks for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants