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

Add filter_glob and exclude_glob to fs.walk #464

Merged
merged 2 commits into from
Jul 12, 2022

Conversation

atollk
Copy link
Member

@atollk atollk commented Mar 22, 2021

Type of changes

  • New feature
  • Documentation / docstrings

Checklist

  • I've run the latest black with default args on new code.
  • I've updated CHANGELOG.md and CONTRIBUTORS.md where appropriate.
  • I've added tests for new code.
  • I accept that @PyFilesystem/maintainers may be pedantic in the code review.

Description

Fixes #371. Fixes #459 .
Added parameters filter_glob and exclude_glob to fs.walk.Walker to perform selection on the walked resources based on their global path.

@coveralls
Copy link

coveralls commented Mar 22, 2021

Coverage Status

Coverage decreased (-0.3%) to 94.794% when pulling d85d9db on atollk:issue_459_371 into 50b1c99 on PyFilesystem:master.

Copy link
Member

@althonos althonos left a comment

Choose a reason for hiding this comment

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

Nice job once again 😉

My largest concern is about whether it would be possible to remove the _translate function completely, and use _PatternMatcher in fs.glob as well to reduce code duplication. Otherwise, it's mostly smaller stuff.

I'll have a closer look at the actual files at some point because GitHub diffs are really not helping here, but the first read didn't raise any red flag 😄

fs/glob.py Outdated Show resolved Hide resolved
fs/glob.py Outdated Show resolved Hide resolved
fs/wildcard.py Outdated Show resolved Hide resolved
fs/wildcard.py Outdated Show resolved Hide resolved
fs/wildcard.py Outdated Show resolved Hide resolved
fs/wildcard.py Outdated Show resolved Hide resolved
fs/base.py Outdated Show resolved Hide resolved
fs/wildcard.py Outdated Show resolved Hide resolved
fs/wildcard.py Outdated Show resolved Hide resolved
@atollk
Copy link
Member Author

atollk commented Apr 30, 2021

Alright, so, after taking a step back and having a look at the larger picture again, it made more sense to me to put all that logic into fs.glob instead of fs.wildcard, so I reverted the change in the latter. I also don't think the two can (or should) be merged anymore, due to the specific behavior of glob. Most notably /**/ matches /, which is not what you would expect from a wildcard.

Either way, several of your previous comments became obsolete through this and I implemented the rest (like partial instead of lambda).

@atollk
Copy link
Member Author

atollk commented May 26, 2021

Hi. Any update on this?

@atollk
Copy link
Member Author

atollk commented Jul 3, 2021

I just rebased this branch onto the newest master, so it's ready to merge now imo.

@althonos
Copy link
Member

althonos commented Jul 6, 2021

@atollk : I think you rebased against your old master without pulling from PyFilesystem/pyfilesystem2 first, the code still have merges from a prior PR.

@atollk
Copy link
Member Author

atollk commented Jul 24, 2021

@althonos Should be fine now.

CHANGELOG.md Outdated Show resolved Hide resolved
fs/base.py Outdated Show resolved Hide resolved
fs/base.py Outdated Show resolved Hide resolved
fs/base.py Show resolved Hide resolved
fs/base.py Show resolved Hide resolved
fs/base.py Outdated Show resolved Hide resolved
Comment on lines +1705 to +1711
patterns (list, optional): A list of patterns, e.g.
``['*.py']``, or `None` to match everything.
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at just the doc-strings, it's hard to see how match_glob() is different from match() ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The summaries (first sentences) differ: Check if a name matches any of a list of wildcards. and Check if a path matches any of a list of glob patterns..

fs/glob.py Outdated Show resolved Hide resolved
Comment on lines +153 to +163
if path and path[0] != "/":
path = "/" + path
Copy link
Contributor

@lurch lurch Jul 25, 2021

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Only with the current implementation of abspath, not with the general idea.

fs/glob.py Outdated Show resolved Hide resolved
fs/glob.py Outdated Show resolved Hide resolved
fs/glob.py Outdated Show resolved Hide resolved
fs/glob.py Outdated Show resolved Hide resolved
fs/walk.py Outdated Show resolved Hide resolved
fs/glob.py Outdated Show resolved Hide resolved
fs/glob.py Show resolved Hide resolved
fs/glob.py Outdated Show resolved Hide resolved
fs/walk.py Outdated Show resolved Hide resolved
fs/walk.py Outdated Show resolved Hide resolved
fs/glob.py Outdated Show resolved Hide resolved
@wayconA
Copy link

wayconA commented Jun 21, 2022

Please promote this PR I'm desperately in need of that feature.
@atollk
@lurch
@althonos

@althonos
Copy link
Member

@wayconA

Sorry for not getting this merged sooner, I've been under lots of work for my research projects.

@atollk has definitely done a lot of work on this, and the reason I've been stalling this PR a bit is probably because I was not 100% happy with the API, but honestly I can't come up with something better, so I will merge this ASAP, and we can always refactor in the pyfilesystem3 release (which I swear is not vaporwave).

@lurch
Copy link
Contributor

lurch commented Jun 22, 2022

Looks like there's a bunch of conflicts that need to be resolved first though 😕

@Wallaku
Copy link

Wallaku commented Jun 26, 2022

What needs to be done for this PR to be approved?
I'm determined to assist if needed.
@atollk
@lurch
@althonos

atollk added 2 commits July 9, 2022 13:06
These extend the class by an option to include/exclude resources by their entire path, not just its last component.
To do so, fs.wildcard had to undergo a rework to remove the dependency on the `re` module.
Unit tests were added for all new/changed code.
@atollk
Copy link
Member Author

atollk commented Jul 9, 2022

done

@Wallaku
Copy link

Wallaku commented Jul 10, 2022

GREAT!!! Thank you very much @atollk

Shall it be merged now?
@althonos @lurch

@althonos althonos merged commit 691deae into PyFilesystem:master Jul 12, 2022
@althonos
Copy link
Member

Great job @atollk, and thanks for the review @lurch.

@Wallaku
Copy link

Wallaku commented Jul 13, 2022

Great! When the new release will be released? @althonos @lurch

@althonos
Copy link
Member

I'd just like #542 to be merged first and then we'll get a new release 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants