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

Provide a stdin-filename to allow stdin to respect force-exclude rules #1780

Merged
merged 10 commits into from Nov 13, 2020
Merged

Provide a stdin-filename to allow stdin to respect force-exclude rules #1780

merged 10 commits into from Nov 13, 2020

Conversation

bellini666
Copy link
Contributor

@bellini666 bellini666 commented Oct 22, 2020

Fix #1776

…ude rules

This will allow automatic tools to enforce the project's
exclude/force-exclude rules even if they pass the file through stdin to
update its buffer.

This is a similar solution to --stdin-display-name in flake8.
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 for the PR @bellini666!

Please update the help output that is in the README, add a test as this is a new feature, and see my other comments below.

Also, personally I find the option name of stdin-filename a bit too specific as you can pass a full absolute filepath. That could be useful for cases where the filename isn't excluded, but its containing directory is excluded. Passing filepaths does break when they would point outside of the current working directory but I dunno how much of an issue that is.

src/black/__init__.py Outdated Show resolved Hide resolved
src/black/__init__.py Outdated Show resolved Hide resolved
@ichard26 ichard26 added the C: configuration CLI and configuration label Oct 22, 2020
@bellini666
Copy link
Contributor Author

@ichard26 I have fixed your comments and also added some tests for the new functionality!

README.md Outdated Show resolved Hide resolved
src/black/__init__.py Outdated Show resolved Hide resolved
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
@bellini666
Copy link
Contributor Author

@hugovk I tried requesting your review again by clicking in the Re-request review next to you on the reviewers section, but for some reason it is not working. Just pinging you here then to say that your suggestions were applied.

@hugovk
Copy link
Contributor

hugovk commented Oct 23, 2020

OK, thanks! That wording looks fine now, I'll leave the functionality to the maintainers to review 👍

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.

README looks good and well done on the tests!

Unfortunately I found another edge case where Black incorrectly uses format_file_in_place.

src/black/__init__.py Outdated Show resolved Hide resolved
@ichard26
Copy link
Collaborator

I completely forgot but please also add an entry in the change log, thank you!
I really need to have a PR review checklist . . .

@hugovk
Copy link
Contributor

hugovk commented Oct 24, 2020

I really need to have a PR review checklist . . .

Or put it in a PR template?
https://docs.github.com/en/free-pro-team@latest/github/building-a-strong-community/creating-a-pull-request-template-for-your-repository

@bellini666 bellini666 changed the title Provide a stdin-filename to allow stdin to respect exclude/force-exclude rules Provide a stdin-filename to allow stdin to respect orce-exclude rules Oct 25, 2020
@bellini666 bellini666 changed the title Provide a stdin-filename to allow stdin to respect orce-exclude rules Provide a stdin-filename to allow stdin to respect force-exclude rules Oct 25, 2020
@bellini666
Copy link
Contributor Author

@ichard26 ok, everything should be fine now :)

Copy link
Collaborator

@cooperlees cooperlees left a comment

Choose a reason for hiding this comment

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

Thanks! Nice addition. Would love @ichard26 to check it one last time before merge tho since they have the most context.

@ichard26
Copy link
Collaborator

ichard26 commented Nov 2, 2020

FYI, I plan on reviewing this soon (as in probably tomorrow).

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.

Overall these changes look good other than the additional bug I found.

Many thanks for working on this!

src/black/__init__.py Outdated Show resolved Hide resolved
src/black/__init__.py Outdated Show resolved Hide resolved
Co-authored-by: Richard Si <63936253+ichard26@users.noreply.github.com>
@bellini666
Copy link
Contributor Author

@ichard26 after applying the suggestions (8d13a9c) some tests broke.

I wasn't able to check it yet, but looking at the commit now, wouldn't that if not is_stdin or p.is_dir(): force to enter the p.is_for() block even if it was supposed to go the the p.is_file() when is_stdin is False? Maybe that line should still be if p.is_dir() alone

@ichard26
Copy link
Collaborator

ichard26 commented Nov 4, 2020

You're correct, my bad. Uh perhaps this patch should work.

diff --git a/src/black/__init__.py b/src/black/__init__.py
index 7594cce..0850b11 100644
--- a/src/black/__init__.py
+++ b/src/black/__init__.py
@@ -633,19 +633,7 @@ def get_sources(
             p = Path(s)
             is_stdin = False
 
-        if not is_stdin or p.is_dir():
-            sources.update(
-                gen_python_files(
-                    p.iterdir(),
-                    root,
-                    include_regex,
-                    exclude_regex,
-                    force_exclude_regex,
-                    report,
-                    gitignore,
-                )
-            )
-        elif is_stdin or p.is_file():
+        if is_stdin or p.is_file():
             normalized_path = normalize_path_maybe_ignore(p, root, report)
             if normalized_path is None:
                 continue
@@ -664,6 +652,18 @@ def get_sources(
                 p = Path(f"{STDIN_PLACEHOLDER}{str(p)}")
 
             sources.add(p)
+        elif p.is_dir():
+            sources.update(
+                gen_python_files(
+                    p.iterdir(),
+                    root,
+                    include_regex,
+                    exclude_regex,
+                    force_exclude_regex,
+                    report,
+                    gitignore,
+                )
+            )
         elif s == "-":
             sources.add(p)
         else:

Didn't give much thought into whether this should work. Tests do pass though. If p is an existing file it will go through the right block. Same thing if --stdin-filename is used regardless if a file or a directory actually exists at the path. If p is an existing directory, well then is_stdin and p.is_file() will be False and the p.is_dir() check will pass. If STDIN mode is used but without --stdin-filename, the elif s == "-" will catch that. Please check this manually before pushing though.

@bellini666
Copy link
Contributor Author

@ichard26 that suggestion worked fine! Just pushed it

@cooperlees
Copy link
Collaborator

Awesome work all! Thanks for your addition to black!

@cooperlees cooperlees merged commit dea81b7 into psf:master Nov 13, 2020
@ichard26 ichard26 removed their request for review February 19, 2021 22:41
noxan pushed a commit to noxan/black that referenced this pull request Jun 6, 2021
psf#1780)

* Provide a stdin-filename to allow stdin to respect exclude/force-exclude rules

This will allow automatic tools to enforce the project's
exclude/force-exclude rules even if they pass the file through stdin to
update its buffer.

This is a similar solution to --stdin-display-name in flake8.

* Update src/black/__init__.py

Co-authored-by: Richard Si <63936253+ichard26@users.noreply.github.com>

* --stdin-filename should only respect --exclude-filename

* Update README with the new --stdin-filename option

* Write some tests for the new stdin-filename functionality

* Apply suggestions from code review

Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>

* Force stdin output when we asked for stdin even if the file exists

* Add an entry in the changelog regarding --stdin-filename

* Reduce disk reads if possible

Co-authored-by: Richard Si <63936253+ichard26@users.noreply.github.com>

* Check for is_stdin and p.is_file before checking for p.is_dir()

Co-authored-by: Richard Si <63936253+ichard26@users.noreply.github.com>
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: configuration CLI and configuration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a --stdin-display-name option like flake8
5 participants