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.

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

Update example exclude to match only files in root #1861

Merged
merged 2 commits into from Jan 27, 2021

Conversation

orn688
Copy link
Contributor

@orn688 orn688 commented Dec 7, 2020

The exclude section of the example pyproject.toml file didn't work as expected. It claimed to exclude matched files only in the project root, but it actually excluded matched files at any directory level within the project. We can address this by prepending ^/ to the regex to ensure that it only matches files in the project root.

See #1473 (comment) for explanation.

The `exclude` section of the example `pyproject.toml` file didn't work
as expected. It claimed to exclude matched files only in the project
root, but it actually excluded matched files at any directory level
within the project. We can address this by prepending `^/` to the regex
to ensure that it only matches files in the project root.

See psf#1473 (comment) for
explanation.
Copy link
Collaborator

@ichard26 ichard26 left a comment

Choose a reason for hiding this comment

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

Looks mostly good, thanks for submitting the PR!

Just a comment error and this should be good to go!

README.md Outdated Show resolved Hide resolved
@orn688
Copy link
Contributor Author

orn688 commented Dec 8, 2020

(sorry for the noise, I'm not very familiar with the GitHub UI and seems like I messed things up by force-pushing)

@orn688 orn688 reopened this Dec 8, 2020
Copy link
Collaborator

@ichard26 ichard26 left a comment

Choose a reason for hiding this comment

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

Thanks!

edit: don’t worry about having messed up things, we all make mistakes, it’s all good :)

@JelleZijlstra JelleZijlstra merged commit 71117e7 into psf:master Jan 27, 2021
(
/(
# 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

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.

JelleZijlstra pushed a commit that referenced this pull request Feb 10, 2021
Resolves #1979 and ensures that the content from #1861 is included in the repository-published documentation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants