Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@orn688 It looks like your explanatory comment here might (accidentally?) be inside the multi-line TOML string that contains the regex.
If that's unintended, it might be worth moving the comment to the line prior to the
exclude
definition.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.
Replying while busy and on a phone so apologies in advance, but include, exclude, and force-exclude regexes are compiled with the verbose flag (
re.compile("your-regex", re.VERBOSE)
) meaning comments are ignored.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.
Take your time, and when you're ready to review more fully: perhaps it would still be worthwhile moving the comment outside of the string, to reduce the escaping complexity?
I'll also note that the
sphinx-build
output differs as a result of placement inside/outside the string; so changes to this file may have other effects.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.
I intentionally put the comment inside the string because, as mentioned, comments are ignored in multilinear regex mode, and because there were other comments within the string already and I find it clearer to keep each comment right next to the bit of the string that it applies to.
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.
Thanks for the explanation @orn688.
My previous concern about output differing based on the string/comment escaping hasn't turned out to be relevant here. It's a long (and no doubt boring) story, but I was using a different markdown parser to perform some local verification checks, and discovered some peculiarities in that tool - but nothing that should affect the documentation build process here.
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.
If it's OK with you both, I'll open a small PR soon to regenerate the
docs
directory and include the root-only-matching comment.It'll also address a subtle repeat occurrence of command-line parameters that crept in when two pull requests individually updated the same documentation file (no merge conflicts occurred; rare but it can happen).
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.
oooo, first time I've seen something like that! Something to watch out for! I should probably add that to the PR review docs this project will hopefully gain soon (see #1759 (comment)). Thanks!
Yeah no issues with me. On my docs overhaul effort, I plan to get rid of the regen step since it's frustrating and annoying without much value given in return, but a regen PR can't hurt. (I guess you could show me more ways to subtlety break a merge lol)
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.
Cool, thanks @ichard26 :) Yep, I saw a bit of the discussion around removing the regen, sounds like a good project.
The way that repeat parameter merge happened was.. kinda confusing to me, to be honest. My theory is that it was some kind of strange loophole in the way that
git
calculates diffs, but it's beyond my brainpower to figure out at the moment. Good to be aware of though for sure.