bpo-25433: Streamline whitespace definitions #14753
Conversation
The previous implementations differed a lot from the the ones found in bytesobject.c and all three of them included hardcoded lists of whitespace characters. Knowledge about whitespace already exists in pyctype.c and should not be duplicated.
As requested in pgo25433, "whitespace" should be implemented and documented only once.
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA). Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. You can check yourself to see if the CLA has been received. Thanks again for your contribution, we look forward to reviewing it! |
There was a problem hiding this comment.
@rompe Welcome and thanks for the contribution!
I would advise closing this PR and splitting it into multiple smaller ones. This usually helps to expedite the review process and to better isolate any build related errors from the integration tests.
The point of failure for travis is occurring on line 1870 of bytearrayobject.c, relating to the auto-generated argument clinic comments. I'm not particularly experienced with using them, so I would recommend checking out the relevant documentation on it if you haven't already.
From a first glance, it also looks like there might be some other whitespace issues, particularly in glossary.rst. Many of these can be resolved through the use of patchcheck. For more information on using it, see the devguide. If that does not auto-correct the spacing issues for you, the integration tests should be able to specify the problematic lines.
On future PRs, I would also advise using the Create draft pull request feature. This allows you to let the integration tests to run without opening the PR, so that you can try to troubleshoot the issues. Optimally, the majority of the troubleshooting is done locally with patchcheck and the automated tests, but this provides final confirmation across multiple platforms. Once the integration tests are passing (or if you are unable to a solution for the test failures), you can select Ready for review, which will change the PR's status to Open. This is not at all required, but it can be quite helpful for both the author and any reviewers.
|
@aeros167 thanks a million for your feedback. I will rework as requested and split code and documentation parts into separate pull requests. (I was under the impression that one bgo issue should equal to one pull request, and I have to admit that I don't understand |
No problem, the docs are quite extensive and we certainly don't expect anyone to know everything. Especially on their first PR to the repository. The process is quite involved and frequently not intuitive at first. |
|
OTOH I have to say that in fact everything is very well documented and I just need more time and practice to absorb it. For everything else Petr has been very helpful at the Europython sprint today, so I think the pull requests should be fine now. |
According to bpo-25433, knowledge about whitespace should be implemented only once and documented only once.
This PR should fulfil both aspects.
https://bugs.python.org/issue25433