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

Download to stdout & cat command #219

Closed
wants to merge 15 commits into from
Closed

Conversation

mjurbanski-reef
Copy link

No description provided.

noxfile.py Outdated
@@ -109,7 +109,7 @@ def install_myself(session, extras=None):
session.run('pip', 'uninstall', 'b2sdk', '-y')
session.run('python', 'setup.py', 'develop')
os.chdir(cwd)
elif CI and not CD:
elif CI and not CD and False:
Copy link
Author

Choose a reason for hiding this comment

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

this is part of last commit that will be discarded before merging upstream

@mjurbanski-reef mjurbanski-reef force-pushed the download_to_stdout branch 3 times, most recently from e21efcb to a00b8d1 Compare November 15, 2023 09:41
requirements.txt Outdated
@@ -1,6 +1,6 @@
argcomplete>=2,<4
arrow>=1.0.2,<2.0.0
b2sdk>=1.24.1,<2
b2sdk @ git+https://github.com/reef-technologies/b2-sdk-python.git@5c21a71d617db1092b35a996505397465fc8d2f4
Copy link
Author

Choose a reason for hiding this comment

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

this is part of last commit that will be discarded before merging upstream

Comment on lines 26 to 27
# Constants used in the B2 API
# TODO B2-47 move API related constants to b2sdk
CREATE_BUCKET_TYPES = ('allPublic', 'allPrivate')

Choose a reason for hiding this comment

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

I believe that commits should address single issue. Is LIST_FILE_NAMES_MAX_LIMIT migration from CLI to sdk related to this PR?

Copy link
Author

Choose a reason for hiding this comment

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

it is not; but since I was here and just so it, and also I'm the one responsible for the other thing I just did it

b2/_utils/uri.py Outdated Show resolved Hide resolved


def _parse_b2_uri(uri, parsed: urllib.parse.ParseResult) -> B2URI | B2FileIdURI:
if parsed.scheme in ("b2", "b2id"):

Choose a reason for hiding this comment

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

Maybe decrease indention?

if parsed.scheme not in ...:
    raise ValueError()
...

b2/arg_parser.py Outdated
Comment on lines 163 to 166
except Exception as e:
if isinstance(e, exc_type):
raise argparse.ArgumentTypeError(translator(e))
raise

Choose a reason for hiding this comment

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

try:
    return func()
except exc_type as e:
    raise argparse.ArgumentTypeError(...)
# no need to catch generic Exception

Comment on lines +1575 to +1580
def run(self, args):
super().run(args)
downloaded_file = self.download_by_b2_uri(args.b2uri, args, '-')
output_filepath = self.get_local_output_filepath('-')
downloaded_file.save_to(output_filepath)
return 0

Choose a reason for hiding this comment

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

This will first download, then output, right? No streaming?

Copy link
Author

Choose a reason for hiding this comment

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

it should absolutely do streaming

I think I propagated not-so-nice name of DownloadedFile from sdk;
where in fact this is just as any other framework streamed response, which is, lets call it "lazy loaded", and the actuall download happens in save_to call

Comment on lines 129 to 132
@pytest.fixture(scope='session')
def monkey_patch():
""" Module-scope monkeypatching (original `monkeypatch` is function-scope) """
from _pytest.monkeypatch import MonkeyPatch

Choose a reason for hiding this comment

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

Now docstring is wrong.

@@ -120,7 +135,7 @@ def monkey_patch():
monkey.undo()


@pytest.fixture(scope='module', autouse=True)
@pytest.fixture(scope='session', autouse=True)
def auto_change_account_info_dir(monkey_patch) -> dir:
"""
Automatically for the whole module testing:

Choose a reason for hiding this comment

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

Docstring is outdated.

Comment on lines 193 to 207
@pytest.fixture(scope='module')
def schedule_bucket_cleanup(global_b2_tool):
"""
Explicitly ask for buckets cleanup after the test

This should be only used when testing `create-bucket` command; otherwise use `bucket_factory` fixture.
"""
buckets_to_clean = set()

def add_bucket_to_cleanup(bucket_name):
buckets_to_clean.add(bucket_name)

yield add_bucket_to_cleanup
for bucket in buckets_to_clean:
global_b2_tool.cleanup_bucket(bucket)

Choose a reason for hiding this comment

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

Well, then bucket_factory should use this fixture to schedule bucket cleanup, no? This way you sorta reuse cleanup code and not duplicate it.

Comment on lines 191 to 196
try:
self.api.delete_bucket(bucket)
except BadRequest:
pass
else: # bucket was empty, we are done
return

Choose a reason for hiding this comment

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

:P

with suppress(BadRequest):
    return self.api.delete_bucket(bucket)

Copy link
Author

Choose a reason for hiding this comment

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

looks weird but I like it :D

@mjurbanski-reef mjurbanski-reef force-pushed the download_to_stdout branch 2 times, most recently from 6dfdabf to 48c1c94 Compare November 15, 2023 18:45
@mjurbanski-reef mjurbanski-reef force-pushed the download_to_stdout branch 5 times, most recently from e9c90f9 to 2a499d5 Compare November 16, 2023 09:59
@mjurbanski-reef
Copy link
Author

merged

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.

5 participants