fix ls --json being corrupted by Using https://... msg
#218
Conversation
mjurbanski-reef
force-pushed
the
fix_stdout_noise
branch
from
0a78850 to
22121b8
Compare
mjurbanski-reef
force-pushed
the
fix_stdout_noise
branch
from
22121b8 to
cec8f22
Compare
test/integration/helpers.py
Outdated
| @@ -337,6 +337,7 @@ def should_equal(expected, actual): | |||
| class CommandLine: | |||
|
|
|||
| EXPECTED_STDERR_PATTERNS = [ | |||
| re.compile(r'^Using https?://[\w.]+.com$'), # account auth | |||
test/unit/test_console_tool.py
Outdated
| @@ -84,7 +84,9 @@ def _run_command_ignore_output(self, argv): | |||
| print('ACTUAL STDERR: ', repr(actual_stderr)) | |||
| print(actual_stderr) | |||
|
|
|||
| self.assertEqual('', actual_stderr, 'stderr') | |||
| assert re.match( | |||
| r'^(|Using https?://[\w.]+.com\n)$', actual_stderr | |||
There was a problem hiding this comment.
I'd prefer (xxx)? over (|xxx) :)
Also, +\.com.
Also, do you need \n if you use $ in the end?
There was a problem hiding this comment.
(|xxx) catches empty line as well as xxx
as for \n - it indeed is not needed
test/unit/test_console_tool.py
Outdated
| ['authorize_account', self.account_id, self.master_key], '', | ||
| "Using http://production.example.com\n", 0 |
There was a problem hiding this comment.
I'd make it consistent with other tests, i.e. add
expected_stderr = """
...
"""
'', expected_sterr, 0
test/unit/test_console_tool.py
Outdated
| ['authorize-account', self.account_id, self.master_key], '', | ||
| 'Using http://custom2.example.com\n', 0 | ||
| ) |
There was a problem hiding this comment.
Same here, make it consistent plz
| self._print_files(args) | ||
| return 0 |
There was a problem hiding this comment.
Why not calling super().run(args)? It won't set args.quiet now, and thus you won't be able to mute self.PROGRESS_REPORT_CLASS later as far as I see.
b2/console_tool.py
Outdated
|
|
||
| @classmethod | ||
| def _print_helper(cls, descriptor, descriptor_encoding, descriptor_name, *args): | ||
| def _print_helper( | ||
| cls, descriptor, descriptor_encoding, descriptor_name, *args, raw: bool = False |
There was a problem hiding this comment.
Why raw instead of print_endline or so?
b2/console_tool.py
Outdated
| self._print( | ||
| json.dumps(data, indent=4, sort_keys=True, cls=B2CliJsonEncoder), enforce_output=True | ||
| ) | ||
| def _print_json(self, data, *, raw: bool = False): |
There was a problem hiding this comment.
This makes no sense to name it _print_json and not to do json dump. I'd say, _print_json should not accept raw data at all. Instead of _print_json(..., raw=True), use _print('[\n', print_endline=False). Actually, since _print(...) adds \n in the end by default, you don't need the print_endline at all, just call _print('['), and it will add \n automatically.
There was a problem hiding this comment.
the reason was that I didn't want to expose enforce_output to anything else that method printing actual data - that would invite someone to break intended --quiet behavior.
I guess I will document it in docstring instead.
| super().run(args) | ||
| def _print_files(self, args): |
There was a problem hiding this comment.
Yeah, that's how it should've been from the beginning.
b2/console_tool.py
Outdated
| super().run(args) | ||
| self._print_files(args) | ||
| if args.json: | ||
| # TODO: Make this work for an infinite generation. | ||
| # Use `_print_file_version` to print a single `file_version` and manage the external JSON list | ||
| # e.g. manually. However, to do it right, some sort of state needs to be kept e.g. info whether | ||
| # at least one element was written to the stream, so we can add a `,` on the start of another. | ||
| # That would sadly lead to an ugly formatting, so `_print` needs to be expanded with an ability | ||
| # to not print end-line character(s). | ||
| self._print_json([file_version for file_version, _ in self._get_ls_generator(args)]) | ||
| return 0 | ||
|
|
||
| return super().run(args) | ||
| if self._first_row: # no files were printed | ||
| self._print_json('[', raw=True) | ||
| self._print_json(']\n', raw=True) | ||
| return 0 |
There was a problem hiding this comment.
I don't get it. How is it supposed to work?
Why not something stupid like
if args.json:
self._print('[')
for i, (file_version, _) in enumerate(self._get_ls_generator(args)):
if i:
self._print(', ', print_endline=False)
self._print_json(file_version)
self._print(']')
else:
self._print_files(args)
|
squash merged in the upstream |
No description provided.