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

Use B2URI in ls & rm #248

Merged
merged 4 commits into from
Feb 2, 2024
Merged

Use B2URI in ls & rm #248

merged 4 commits into from
Feb 2, 2024

Conversation

emnoor-reef
Copy link

@emnoor-reef emnoor-reef commented Jan 29, 2024

Too much inheritance is making my head spin πŸ˜΅β€πŸ’«

@emnoor-reef emnoor-reef reopened this Jan 31, 2024
Comment on lines 1 to 19
######################################################################
#
# File: b2/_internal/b2v3/rm.py
#
# Copyright 2023 Backblaze Inc. All Rights Reserved.
#
# License https://www.backblaze.com/using_b2_code.html
#
######################################################################
from __future__ import annotations

from b2._internal._b2v4.registry import B2URIBucketNFolderNameArgMixin, BaseRm


# NOTE: We need to keep v3 Rm in separate file, because we need to import it in
# unit tests without registering any commands.
class Rm(B2URIBucketNFolderNameArgMixin, BaseRm):
__doc__ = BaseRm.__doc__
# TODO: fix doc
Copy link
Author

Choose a reason for hiding this comment

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

it works, but feels like a stupid idea.

Choose a reason for hiding this comment

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

I think I implemented this stupid idea first in this project, and IMO if it works and there is not clear reason not to use it then why not :P

@emnoor-reef emnoor-reef marked this pull request as ready for review February 1, 2024 08:03
@emnoor-reef
Copy link
Author

Fixed the examples of ls and rm in docstrings.

@@ -2477,6 +2464,7 @@ def _setup_parser(cls, parser):
)
parser.add_argument('--noProgress', action='store_true')
parser.add_argument('--failFast', action='store_true')
# TODO: --bypassGovernance

Choose a reason for hiding this comment

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

What's this?

Copy link
Author

Choose a reason for hiding this comment

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

It was there to remind me to implement --bypassGovernance for rm in a future PR, but forgot to remove it. :P

@@ -0,0 +1 @@
Change `ls` and `rm` commands to use the `b2://` URI scheme.

Choose a reason for hiding this comment

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

Maybe mention that this is only for the pre-release _b2v4 executable, as not to imply a breaking change?

B2_URI_FILE_ARG_TYPE = wrap_with_argument_type_error(b2_file_uri)


def add_b2_uri_argument(parser: argparse.ArgumentParser, name="B2_URI"):
"""
Add a B2 URI pointing to a file as an argument to the parser.

Choose a reason for hiding this comment

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

The doc copied from the other func is probably inaccurate.

Comment on lines +41 to 42
B2_URI_ARG_TYPE = wrap_with_argument_type_error(b2_uri)
B2_URI_FILE_ARG_TYPE = wrap_with_argument_type_error(b2_file_uri)

Choose a reason for hiding this comment

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

When we have this distinction of URI vs URI_FILE, this naming is a bit misleading (at least it did mislead me a bit). That's because the real distinction here is not "file" vs "non-file" (i.e. a folder, a pattern), it's actually between file id vs any URI (b2:// vs b2id://). It might be worth renaming B2_URI_FILE_ARG_TYPE to something more explicit.

Same goes for add_b2_file_argument (-> add_b2_file_id_argument). I know this these are not your names, it's just that seeing b2_uri etc. brings this to attention. :)

Copy link
Author

Choose a reason for hiding this comment

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

Yup, it is unclear if the names mean b2://, b2id:// or both. I'd leave them like that for now. I'll clean them up in the follow up PR adding b2id:// support to rm.

@emnoor-reef emnoor-reef merged commit 8122bb6 into reef-technologies:master Feb 2, 2024
27 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