Add a copyright checker as a pylint plugin #4577

yjt98765 · 2021-10-15T10:55:34Z

This PR implements a pylint checker that automatically checks the copyright notice at the beginning of a python file. This task is proposed in Issue 4564.

The PR consists of the following parts:

The implemented pylint checker (dev_tools/pylint_copyright_checker.py) and its test file (dev_tools/pylint_copyright_checker_test.py). The checker reports a message with id "R0001" if there is no copyright notice or the notice does not conform to the standard one. The letter 'R' represents refactoring. The checking can be disabled in the file with pragma # pylint: disable=wrong-copyright-notice
Modification of pylint-related files (check/pylint, check/pylint-changed-files, dev_tools/conf/.pylintrc) to integrate the new checker into the workflow.
Python files that failed in this check. Some copyright notices were wrong (e.g. cirq-core/cirq/circuits/circuit_dag.py and cirq-core/cirq/ops/qubit_order_test.py), and they are corrected. Some files did not have a copyright notice. In this case, the "disable" pragma is added. Note that not all the files need to have a copyright notice, probably. After the modification, these files can pass pylint checking again.

Also modify existing Python files that violate the copyright checking.

google-cla · 2021-10-15T10:55:43Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

It is required by the copyright checker.

MichaelBroughton

Thanks so much for opening this! This is a good looking change. Few high level comments.

MichaelBroughton · 2021-10-19T21:42:54Z

cirq-google/cirq_google/engine/client/quantum.py

+#
+# -*- coding: utf-8 -*-


What are these coding: utf-8 entries for ?

I do not think it is needed (anymore), but it is originally in the head. That line violates the copyright checking rule. Therefore, I move it here, below the copyright. We can delete it if it is of no use. There are also other files sharing the same situation.

The original entry in the first line complies with Python's standard of declaring the source file encoding. However, the default encoding of Python 3 has become utf-8. Therefore, the declarations in this project are unnecessary. I will remove all such comments in the next commit. If we need them back, they can also be unrolled. Some autogenerated files (such as *_pb2.py) also have this line in the beginning. I will not touch them.

MichaelBroughton · 2021-10-19T22:36:43Z

dev_tools/pylint_copyright_checker.py

+            for (lineno, line) in enumerate(stream):
+                if lineno >= len(golden):
+                    break
+
+                if lineno == 0:
+                    # Use Regex to accept various years such as 2018 and 2021.
+                    if not re.compile(golden[0]).match(line):
+                        self.add_message("wrong-copyright-notice", line=1)
+                        break
+                elif line.rstrip() != golden[lineno]:
+                    self.add_message("wrong-copyright-notice", line=lineno + 1)
+                    break


Can probably tighten this up a bit with:

for expected_line, (lineno, line) in zip(golden, enumerate(stream)): if not re.match(expected_line, line): self.add_message("wrong-copyright-notice", line=lineno + 1) break

And use regex on every line so that we can replace doing rstrip() and lstrip() equality checks with something more like: ^expected stuff$. Also, a quick look at the API for add_message and I think we might be able to add a col_offset as well so the user knows what column the disagreement is on as well as the line number (this might make using regex hard).

Thank you for the suggestions. Indeed, it would be difficult to use Regex to report a position. I looked for some ways (such as this). They all seem too complex for this task. I am also worried about the performance since running pylint on the whole project already takes quite a long time. Therefore, I choose to use plain byte comparison, which might be stricter than regex solutions. However, it is probably fine. For example, there is no point to have some whitespaces before the #. Nevertheless, the right part of a line should have more flexibility. For example, Windows and Linux have different line endings and whitespaces cannot been seen in the editor. Therefore, I apply strip() to the part of the file line longer than the template. As long as the longer part consists of only whitespace, it will be allowed.

Unfortunately, the test utility provided by pylint does not support the col_offset feature. Therefore, it is not tested in the test file. I have tried it with several different cases manually, and it works as expected.

MichaelBroughton · 2021-10-19T23:06:56Z

check/pylint

@@ -13,4 +13,5 @@ cd "$(git rev-parse --show-toplevel)"

 CIRQ_MODULES=$(env PYTHONPATH=. python dev_tools/modules.py list --mode package-path)

-pylint --rcfile=dev_tools/conf/.pylintrc $@ $CIRQ_MODULES dev_tools examples


You might want to add this change to pylint-incremental as well.

Thanks. Added it.

MichaelBroughton · 2021-10-19T23:44:16Z

dev_tools/requirements/deps/pylint.txt

+pylint~=2.6.0
+astroid<2.5


It looks like pylint already has this here: https://github.com/PyCQA/pylint/blob/2.6/pylint/__pkginfo__.py#L43

Do we need this ?

I did not have this in commit 6a2937a, which fails the CI process. The report says E ModuleNotFoundError: No module named 'astroid'. Maybe adding this can solve this problem?

It looks like that failure is caused by pylint not getting explicitly installed. That CI test triggers this dev_tools/conf/pip-install-minimal-for-pytest-changed-files.sh. Which does not include the pylint install which is now a requirement for dev tools testing. With that in mind I would suggest we try adding a line like -r pylint.txt to pytest.txt with a comment explaining that in order to test this custom plugin, we need pylint installed.

I see. So in the new commit, I added -r pylint.txt to pip-install-minimal-for-pytest-changed-files.sh and removed astroid from pylint.txt. Hope it works :)

yjt98765 · 2021-10-20T00:38:30Z

cirq-google/cirq_google/engine/client/quantum.py

@@ -1,5 +1,3 @@
-# -*- coding: utf-8 -*-


Note this line: coding: utf8-8

Also remove some encoding declarations.

vtomole · 2021-10-21T01:57:36Z

Let's have pylint: disable=wrong-or-nonexistent-copyright-notice if that's not too long.

CirqBot · 2021-10-31T20:03:00Z

Automerge cancelled: A required status check is not present.

Missing statuses: ['cla/google']

vtomole · 2021-10-31T20:05:29Z

@yjt98765 Will https://stackoverflow.com/a/58190576/5716192 work?

yjt98765 · 2021-11-01T01:55:39Z

@yjt98765 Will https://stackoverflow.com/a/58190576/5716192 work?

Thanks. It triggers the CI, and the cla check passed!

CirqBot · 2021-11-01T02:51:27Z

Automerge cancelled: A required status check is not present.

Missing statuses: ['Build docs', 'Build protos', 'Changed files test', 'Coverage check', 'Doc test', 'Format check', 'Lint check', 'Misc check', 'Notebook formatting', 'Pytest MacOS (3.7)', 'Pytest MacOS (3.8)', 'Pytest MacOS (3.9)', 'Pytest Ubuntu (3.7)', 'Pytest Ubuntu (3.8)', 'Pytest Ubuntu (3.9)', 'Pytest Windows (3.7)', 'Pytest Windows (3.8)', 'Pytest Windows (3.9)', 'Type check', 'Typescript lint check', 'Typescript tests', 'Typescript tests coverage']

CirqBot · 2021-11-01T03:08:01Z

Automerge cancelled: Merging is blocked (I don't understand why).

Comment's addressed.

vtomole · 2021-11-01T03:13:57Z

@yjt98765 Merged. Thanks for your patience!

This PR implements a pylint checker that automatically checks the copyright notice at the beginning of a python file. This task is proposed in [Issue 4564](quantumlib#4564). The PR consists of the following parts: 1. The implemented pylint checker (dev_tools/pylint_copyright_checker.py) and its test file (dev_tools/pylint_copyright_checker_test.py). The checker reports a message with id "R0001" if there is no copyright notice or the notice does not conform to the standard one. The letter 'R' represents refactoring. The checking can be disabled in the file with pragma `# pylint: disable=wrong-copyright-notice` 2. Modification of pylint-related files (check/pylint, check/pylint-changed-files, dev_tools/conf/.pylintrc) to integrate the new checker into the workflow. 3. Python files that failed in this check. Some copyright notices were wrong (e.g. cirq-core/cirq/circuits/circuit_dag.py and cirq-core/cirq/ops/qubit_order_test.py), and they are corrected. Some files did not have a copyright notice. In this case, the "disable" pragma is added. Note that not all the files need to have a copyright notice, [probably](https://softwareengineering.stackexchange.com/a/19653/322605). After the modification, these files can pass pylint checking again.

yjt98765 added 3 commits October 14, 2021 15:32

Add copyright checker plugin for Pylint.

33012c8

Add test file for the copyright checker.

cc48d96

Also modify existing Python files that violate the copyright checking.

Merge branch 'master' of https://github.com/yjt98765/Cirq

aa3615d

yjt98765 requested review from cduck, ColemanCollins, dabacon, gmauricio, mrwojtek, nakardo, vtomole, wcourtney and a team as code owners October 15, 2021 10:55

yjt98765 requested a review from maffoo October 15, 2021 10:55

CirqBot added the size: L 250< lines changed <1000 label Oct 15, 2021

google-cla bot added the cla: no label Oct 15, 2021

Merge branch 'master' into master

b3cf8b1

google-cla bot added cla: yes Makes googlebot stop complaining. and removed cla: no labels Oct 16, 2021

yjt98765 added 4 commits October 17, 2021 09:14

Merge branch 'master' into master

6a2937a

Merge branch 'quantumlib:master' into master

5057aa3

Add astroid to package dependency list.

0b357d1

It is required by the copyright checker.

Merge branch 'master' into master

c4c3149

MichaelBroughton self-assigned this Oct 19, 2021

MichaelBroughton previously requested changes Oct 19, 2021

View reviewed changes

yjt98765 commented Oct 20, 2021

View reviewed changes

yjt98765 added 3 commits October 20, 2021 12:32

Add col_offset attribute to the report message.

8301f1f

Also remove some encoding declarations.

Merge branch 'master' of https://github.com/yjt98765/Cirq

113da1b

Require explicitly installing pylint in the workflow

07e4919

CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Oct 31, 2021

CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Oct 31, 2021

yjt98765 added 2 commits November 1, 2021 09:42

Merge branch 'master' into master

c5f0206

Merge branch 'master' of https://github.com/yjt98765/Cirq

08861e9

vtomole added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Nov 1, 2021

CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Nov 1, 2021

CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Nov 1, 2021

vtomole added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Nov 1, 2021

CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Nov 1, 2021

CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Nov 1, 2021

vtomole added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Nov 1, 2021

CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Nov 1, 2021

CirqBot merged commit 1bb7554 into quantumlib:master Nov 1, 2021

CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Nov 1, 2021

yjt98765 mentioned this pull request Dec 16, 2021

Add JSON serialization support for QasmUGate #4750

Merged

mhucka mentioned this pull request Jul 31, 2024

pylint fails with errors about bad-option-value #6691

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a copyright checker as a pylint plugin #4577

Add a copyright checker as a pylint plugin #4577

yjt98765 commented Oct 15, 2021 •

edited

Loading

google-cla bot commented Oct 15, 2021

MichaelBroughton left a comment

MichaelBroughton Oct 19, 2021

yjt98765 Oct 20, 2021

yjt98765 Oct 20, 2021 •

edited

Loading

MichaelBroughton Oct 19, 2021 •

edited

Loading

yjt98765 Oct 20, 2021

MichaelBroughton Oct 19, 2021

yjt98765 Oct 20, 2021

MichaelBroughton Oct 19, 2021

yjt98765 Oct 20, 2021

MichaelBroughton Oct 20, 2021

yjt98765 Oct 21, 2021

yjt98765 Oct 20, 2021

vtomole commented Oct 21, 2021

CirqBot commented Oct 31, 2021

vtomole commented Oct 31, 2021

yjt98765 commented Nov 1, 2021

CirqBot commented Nov 1, 2021

CirqBot commented Nov 1, 2021

vtomole commented Nov 1, 2021

		@@ -13,4 +13,5 @@ cd "$(git rev-parse --show-toplevel)"

		CIRQ_MODULES=$(env PYTHONPATH=. python dev_tools/modules.py list --mode package-path)

		pylint --rcfile=dev_tools/conf/.pylintrc $@ $CIRQ_MODULES dev_tools examples

		pylint~=2.6.0
		astroid<2.5

Add a copyright checker as a pylint plugin #4577

Add a copyright checker as a pylint plugin #4577

Conversation

yjt98765 commented Oct 15, 2021 • edited Loading

google-cla bot commented Oct 15, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

MichaelBroughton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yjt98765 Oct 20, 2021 • edited Loading

Choose a reason for hiding this comment

MichaelBroughton Oct 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtomole commented Oct 21, 2021

CirqBot commented Oct 31, 2021

vtomole commented Oct 31, 2021

yjt98765 commented Nov 1, 2021

CirqBot commented Nov 1, 2021

CirqBot commented Nov 1, 2021

vtomole commented Nov 1, 2021

yjt98765 commented Oct 15, 2021 •

edited

Loading

yjt98765 Oct 20, 2021 •

edited

Loading

MichaelBroughton Oct 19, 2021 •

edited

Loading