-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
FIX: Regression of not excluding files from build #1606
FIX: Regression of not excluding files from build #1606
Conversation
@finswimmer Shouldn't this be based on |
Maybe? :) Because my PR #1464 was based on develop I did here the same way. Any easy way to switch? |
@finswimmer Oh sorry, I thought it was a bug fix but it was actually a regression fix for your previous PR so |
@finswimmer I changed the base branch to |
@sdispater: That was a hard work doing my first rebase. But now I got it (I think) 🚀 |
@finswimmer I don't think your rebase went well since this PR includes commits that are not relevant to your changes. You only need to rebase your 4 commits onto |
b62d772
to
b3ca03d
Compare
…mplete list of excluded files. Instead when checking if a file is excluded, it is looked up if one of the parent folder is in the excluded list.
…ad to infinite loop
b3ca03d
to
c73e19f
Compare
TIL: Never ever (never!) use the wrong branch! Better now @sdispater ? |
filepath = filepath.as_posix() | ||
exclude_path = Path(filepath) | ||
|
||
while True: |
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.
Why not while len(exclude_path.parts) > 1
?
I try to avoid while True
statements to have a more explicit condition.
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.
Because - at least in theory - the root folder could be excluded as well. (Which doesn't make sense, if you want to build a package, but people have sometimes strange ideas).
Of course I could check this if I leave the loop. If you would like this more, I could implement it.
For things like this a native do-while implementation in python would be nice. But AFAIK there isn't such a thing 😞 .
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.
You know what? We can leave it like this for now. I don't think this is too much of an issue.
Can confirm the fix works. Thanks! |
* FIX: To find excluded files, no additional glob is used to build a complete list of excluded files. Instead when checking if a file is excluded, it is looked up if one of the parent folder is in the excluded list. * making isort happy * revert accidentally made changes to pyproject.toml/poetry.lock * fix: change condition for breaking loop, as previous version could lead to infinite loop
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This PR changes the way, how it is checked if a file should be excluded from build.
Fixes: #1597
Pull Request Check List
This is just a reminder about the most common mistakes. Please make sure that you tick all appropriate boxes. But please read our contribution guide at least once, it will save you unnecessary review cycles!