PEP 674: Leave PyDescr_TYPE() unchanged #2277
Conversation
|
Oh. I didn't expect so many reviews :-D @JelleZijlstra, @hugovk, @AA-Turner: Would you mind to review again the change? |
CAM-Gerlach
left a comment
CAM-Gerlach
left a comment
There was a problem hiding this comment.
Thanks! Just some minor grammar/phrasing and clarity issues.
pep-0674.rst
Outdated
|
|
||
| At December 1, 2021, a code search on the PyPI top 5000 projects (4760 | ||
| At January 26, 2022, a code search on the PyPI top 5000 projects (4762 | ||
| projects in practice, others don't have a source archive) found that |
There was a problem hiding this comment.
| projects in practice, others don't have a source archive) found that | |
| projects in practice; others don't have a source archive) found that |
Comma splice (grammar error)
|
Hey @vstinner I will re-review to cover anything you just added and update a few suggestions that got outdated due to the commit in the middle of my review, but just as a tip especially in situations like this, it if you batch-apply the GitHub suggestions we all made (even if then squash/fixup that commit into another), it makes it easier for reviewers since it auto-resolves and collapses the comments and avoids the need to manually re-review to check that they got applied correctly. Thanks! |
I collapsed all the ones Victor addressed |
| Only 13 out of the top 5000 PyPI projects (0.3%) are affected by 2 macro | ||
| changes. Moreover, 22 projects just have to regenerate their Cython | ||
| code. |
There was a problem hiding this comment.
| Only 13 out of the top 5000 PyPI projects (0.3%) are affected by 2 macro | |
| changes. Moreover, 22 projects just have to regenerate their Cython | |
| code. | |
| Only 13 out of the top 5000 PyPI projects (0.3%) are affected, and only by a | |
| total of two of the macro changes. An additional 22 projects just have to | |
| regenerate their Cython code. |
Updating this suggestion to reflect the changes, and adding one other fix:
- As a reader of the PEP, it confused me as to how there could be 22 projects that had to re-Cythonize, but only 13 that were affected.
- Additionally, the wording surrounding the "two macro changes" was potentially unclear and could imply something other than you mean
| * zstd (1.5.0.2) | ||
|
|
||
| These 14 projects only use 4 macros as l-value: | ||
| Of these 13 projects, only 2 macros are used as an l-value: |
There was a problem hiding this comment.
| Of these 13 projects, only 2 macros are used as an l-value: | |
| In these 13 projects, only 2 macros are used as an l-value: |
Grammar
|
Too many comments, latest comments are on outdated commits, so it's very confusing. I merged my PR. Thanks for reviews. |
|
Hey @vstinner , sorry for the confusion and the repeated rounds of suggestions by multiple reviewers, and thanks for bearing with us. As a tip for the future, batch-applying the GitHub suggestions reviewers have left for you through GitHub's UI, as intended, will help avoid most of the issues and frustration on this PR, and make things easier for both you and reviewers in the future. Also, if you are going to make additional changes on a PR before it is ready for review and merge, using the "Draft" status when opening it or clicking "Convert to draft" below the reviewers list clearly signals to us to hold off on reviewing (or merging) it until you're ready for it and don't review to-be-outdated commits, avoiding extra work for both you and us. It looks like only two changes (not mooted by #2280 ) were not in the merged PEP (both on lines touched by #2280, but that was merged before I submitted my review with those two suggestions). Neither are critical, and the PEP could use a copyediting/proofreading pass anyway, so I'll take care of that in a separate PR when I get to it—better than piling on more suggestions here, for sure 😄 |
6 participants
No description provided.