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 --include and --exclude filters to the ls and rm commands #253

Merged
merged 11 commits into from
Feb 22, 2024
Merged

Add --include and --exclude filters to the ls and rm commands #253

merged 11 commits into from
Feb 22, 2024

Conversation

emnoor-reef
Copy link

No description provided.

@@ -165,10 +165,10 @@ def list_file_versions_by_uri(self, uri, *args, **kwargs):
raise NotImplementedError(f"Unsupported URI type: {type(uri)}")

@list_file_versions_by_uri.register
def _(self, uri: B2URI, *args, **kwargs):
def _(self, uri: B2URI, *args, filters=(), **kwargs):

Choose a reason for hiding this comment

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

Type hint?

Comment on lines 2179 to 2180
parser.add_argument('--include', dest='filters', action='append', type=Filter.include)
parser.add_argument('--exclude', dest='filters', action='append', type=Filter.exclude)

Choose a reason for hiding this comment

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

The docstring is outdated now.

@@ -2198,6 +2201,7 @@ def _get_ls_generator(self, args):
latest_only=not args.versions,
recursive=args.recursive,
with_wildcard=args.withWildcard,
filters=args.filters or (),

Choose a reason for hiding this comment

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

Maybe use default value in add_argument instead? Is this possible?

parser.add_argument('--include', dest='filters', action='append', type=Filter.include, default=())

Copy link
Author

@emnoor-reef emnoor-reef Feb 9, 2024

Choose a reason for hiding this comment

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

That doesn't work. With action='append', argparse stores the default, copies the object with dest[:] before appending, then calls .append() on that. So tuples don't work here.

But we can have default=[] here. A quick forage through argparse code verifies that having multiple flags with with the same destination and a default won't cause any problems.

@@ -2445,6 +2445,64 @@ def test_ls_b2id(self):
'''
self._run_command(['ls', f'b2id://{file_version.id_}'], expected_stdout, '', 0)

def test_ls_filters(self):

Choose a reason for hiding this comment

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

This checks filters working on ls command but doesn't ensure they work with rm command. I understand that it should work automatically bc of some inheritance, but still I think some very basic test would be good here. If some code changes in future, we don't want rm to misbehave with those flags.

@emnoor-reef emnoor-reef merged commit 6e0c2e2 into reef-technologies:master Feb 22, 2024
30 checks passed
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.

3 participants