| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
|
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? |
Sorry, something went wrong.
|
@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! |
Sorry, something went wrong.
|
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). |
Sorry, something went wrong.
There was a problem hiding this comment.
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.)
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
🤭 I see now. The commits have been updated. |
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM now; thanks @oraluben !
Sorry, something went wrong.
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 |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
No description provided.