Closed cornelius closed 5 years ago
Hello @cornelius! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
contrib/devtools/copyright_header.py
:Line 319:1: E305 expected 2 blank lines after class or function definition, found 1
I extracted the scripted diff now in #1072.
This pull request only contains the manual changes and the changes to the script.
The last commit just touched one of the involved regular expressions, the one I pointed out should be modified too.
The last commit just touched one of the involved regular expressions, the one I pointed out should be modified too.
@castarco Thinking about this again, I'm not sure if making the check more relaxed is the right path to go. We have two functionalities here:
The first is copyright_header.py report
to analyze the copyright headers of the code. This will report files with missing copyright headers and unknown copyright holders. This is helpful to get the copyright headers into the right form in the first place. From this point of view I think it actually would be better, if it would not accept "Copyright(c)" as a valid header, but report it as missing. Then we see this and can manually fix it. This results in a more uniform style for the headers which is easier to read and to analze.
The second functionality is copyright_header.py
which updates the years in the copyright headers. This assumes a certain style of the headers. As this only concerns our own copyrights and we can easily make the code use a consistent style, this assumption can be relatively strict. We can make sure that the assumption holds by running the reports and fixing the issues it reports.
So I would actually suggest to revert to the original regexp for the check, so that the copyright notice is reported and can be fixed, and we don't have to support multiple styles of how to format the copyright headers.
The first is
copyright_header.py report
to analyze the copyright headers of the code. This will report files with missing copyright headers and unknown copyright holders. This is helpful to get the copyright headers into the right form in the first place. From this point of view I think it actually would be better, if it would not accept "Copyright(c)" as a valid header, but report it as missing. Then we see this and can manually fix it. This results in a more uniform style for the headers which is easier to read and to analze.The second functionality is
copyright_header.py
which updates the years in the copyright headers. This assumes a certain style of the headers. As this only concerns our own copyrights and we can easily make the code use a consistent style, this assumption can be relatively strict. We can make sure that the assumption holds by running the reports and fixing the issues it reports.
Hmmm... But this does not mean that whenever the script replaces the values will keep the previous style, right. I mean, this will detect various formats, and then replace whatever finds by a fixed structure, or might be that only applies changes whenever there's a difference on the dates?
Hmmm... But this does not mean that whenever the script replaces the values will keep the previous style, right. I mean, this will detect various formats, and then replace whatever finds by a fixed structure, or might be that only applies changes whenever there's a difference on the dates?
It replaces what it finds by a fixed structure if the data needs to be updated, but what it looks for is identical to the fixed structure, modulo the dates. We could change that to also update the structure if it's formatted differently, so not only updating the date but also fixing formatting, but I'm not sure if we should mix these two things. It might be better to introduce a lint check which checks pull requests for proper copyright headers.
Also there can be various ways how the formatting of the headers can go wrong and automatically detecting that seems to be a bit of an overkill given that it's relatively rare that files with malformed headers get added.
Concept ACK 2db29ec44ad064ebf49339152f6ce8f1200e175e
copyright_headers.py
to not take merge commits into account when calculating the date of the last changecopyright_headers.py
copyright_headers.py update
test/functional/test_framework/blockstore.py