Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Update example exclude to match only files in root #1861

Merged
merged 2 commits into from
Jan 27, 2021
Merged

Update example exclude to match only files in root #1861

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6c9e51e
Update example exclude to match only files in root
orn688 Dec 7, 2020
1c0ebe6
Mention excluding directories as well
orn688 Dec 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 4 additions & 3 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,9 +308,10 @@ line-length = 88
target-version = ['py37']
include = '\.pyi?$'
exclude = '''

(
/(
# A regex preceded with ^/ will apply only to files and directories
# in the root of the project.
Copy link
Contributor

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.

Copy link
Collaborator

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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).

Copy link
Collaborator

@ichard26 ichard26 Feb 9, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

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!

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.

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)

Copy link
Contributor

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.

^/(
(
\.eggs # exclude a few common directories in the
| \.git # root of the project
| \.hg
Expand Down