-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
PEP 13 and PEP 245: Fix incorrect PEP references #2267
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @oraluben ! This looks good; can we go ahead and merge?
Technically, its a PEP 13 change, but its to fix a trivial, unambiguous regression that was introduced by the syntax change in #2209 , so given it is effectively a revert for a critical bug, it seems reasonable to merge after review and approval at the PEP editor level.
Hi @CAM-Gerlach, I noticed there are some footnotes whose numbers haven't been updated after previous entities have been deleted, is that expected/valid? |
@oraluben If both the footnote and the references to it were left unchanged, then the footnote should still work, the numbers just won't be sequential—which is bothersome, but may or may not be worth the changing (I'd lean yes, personally, but I'd have to see the specific instances). If one but not the other was changed, then the footnotes will be broken (you can confirm on the live site), which is a bug and should certainly be fixed. If you could list/link them here in two separate categories, critical (i.e. footnotes/links/etc. point to the wrong target or don't work at all) and non-critical (e.g. footnotes are not sequentially numbered) that would be much appreciated. Thanks! |
Hi @CAM-Gerlach, I've finished walking through #2209, and hopefully I've got all critical issues (in the first two commits, about PEP 13 & 245). It seems unlikely I can sequence all footnotes immediately, there's a large amount of them. Please let me know about your plan / what I should do about this PR (e.g. drop the third commit, change commit message). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, if you could just drop acbc9ef where you do the trivial changes and we can go ahead with the two critical ones. Thanks.
(NB, the reason I specifically asked to list instead of commit the changes was doing the latter would ping a bunch of codeowners unnecessarily; fortunately, only one plus Brett ended up being pinged here.)
As for the commit messages, by standard convention commit summaries should be treated like titles, so not only are periods redundant but should not be used. But don't worry about that now; we squash every PR anyway so I can fix it there, thanks. |
acbc9ef
to
ef49d92
Compare
🤭 I see now. The commits have been updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now; thanks @oraluben !
I updated some, but it was very labour-intensive. It would be a better use of time to inline the footnotes to links, if this is going to be done. (If we should or not is a distinct matter, and deserves its own issue.) Thanks @oraluben for catching these. (I'm slightly pleased that you only found two critical issues, although annoyed there were any at all!) A |
Yeah, no argument there. At present, based on our discussion in #2130 , I think its worthwhile to do it at some point in the subset of Active/Process (Meta-)PEPs that are kept updated and frequently read and referred to, since it substantially improves usability for the many readers they get and is consistent with how future PEPs are expected to be written, but IMO its too noisy and not a worthwhile use of time to do it for all the rest, mostly older PEPs that are of more historical value anyway. |
No description provided.