Support '-' as STDOUT alias when download files #211

athakur-reef · 2023-10-17T09:00:54Z

No description provided.

mjurbanski-reef · 2023-10-17T09:56:28Z

what about
``

support - as /dev/stdout alias in CLI IF no file with such name exists in current directory (same way we do it for - input)

athakur-reef · 2023-10-17T12:57:22Z

Support - as /dev/stdout alias.

Used CON for stdout at Windows like /dev/stdout at Linux

b2/console_tool.py

athakur-reef · 2023-10-17T16:34:19Z

Used updated save_to method for download
Warn user about deprecation of this like we do on stdin alias

mjurbanski-reef

see https://github.com/reef-technologies/b2-sdk-python/pull/309/files/4916ab1f1cd818711e7b44cd685a86995b0eae47#diff-41b295fa3878a7f108434f848eadaf27760e8dc8b4a4bde9cf4750ca57b91583

otherwise PR for CLI seems good :)

mjurbanski-reef · 2023-10-17T17:22:37Z

CHANGELOG.md

+### Fixed
+
+* Fix to support '-' as stdout alias


this is new feature and not a fix

athakur-reef · 2023-10-17T19:09:58Z

Change the CLI to provide stdout without changing the SDK

ppolewicz

I think both PRs missed an important thing: why and when we use wb+ vs wb. That's messed up and was messed up for years now for no good reason. We have a chance to fix it now but it doesn't seem that we are taking it.

ppolewicz · 2023-10-17T21:19:16Z

CHANGELOG.md

@@ -6,6 +6,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

 ## [Unreleased]

+### Added
+


this line should not be there

CHANGELOG.md

ppolewicz · 2023-10-17T21:21:04Z

b2/console_tool.py

+    MaxDownloadStreamsMixin,
+    DownloadCommand,
+):
+    def get_download_info_from_local_filename(self, filename: str):


Suggested change

def get_download_info_from_local_filename(self, filename: str):

def _get_download_info_from_local_filename(self, filename: str):

the convention in this file is to prefix protected method names with a _

ppolewicz · 2023-10-17T21:21:58Z

b2/console_tool.py

+    DownloadCommand,
+):
+    def get_download_info_from_local_filename(self, filename: str):
+        stdout_file_path = 'CON' if platform.system() == 'Windows' else '/dev/stdout'


that should be a (class) const so that it's computed once per program execution and not on every single download call. Here it doesn't matter much because CLI use is one-shot, but generally in terms of how you arrange the code, this should be a constant

ppolewicz · 2023-10-17T21:37:24Z

b2/console_tool.py

+            return stdout_file_path, 'wb', False
+
+        allow_seeking = filename != stdout_file_path
+        mode = 'wb+' if allow_seeking else 'wb'


I think we should be using the same thing for named pipes and it should generally be not CLI but sdk

Fix the stdout_file_path as const of CLASS

ppolewicz · 2023-10-19T02:26:42Z

CHANGELOG.md

 ### Fixed
 * `--quiet` now will implicitly set `--noProgress` option as well
+* Fix failing open non-seekable file


there is something wrong grammaticaly with this line

ppolewicz · 2023-10-19T02:27:07Z

CHANGELOG.md

@@ -6,8 +6,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

 ## [Unreleased]

+### Deprecated
+* Support of `-` as a valid filename in `download-file-by-name` or `download-file-by-id` command. In future `-` will be an alias for standard output.


I think people typically write "in THE future", right?

ppolewicz · 2023-10-19T02:29:57Z

b2/console_tool.py

+        if filename == '-':
+            if os.path.exists('-'):
+                self._print_stderr(
+                    "WARNING: Filename `-` won't be supported in the future and will be treated as stdout alias."
+                )
+                return filename
+            return self.STDOUT_FILE_PATH
+        return filename


Suggested change

if filename == '-':

if os.path.exists('-'):

self._print_stderr(

"WARNING: Filename `-` won't be supported in the future and will be treated as stdout alias."

)

return filename

return self.STDOUT_FILE_PATH

return filename

if filename != '-':

return filename

if not os.path.exists(filename):

return self.STDOUT_FILE_PATH

self._print_stderr(

"WARNING: Filename `-` won't be supported in the future and will be treated as stdout alias."

)

return filename

ppolewicz · 2023-10-19T02:34:35Z

b2/console_tool.py

+):
+    STDOUT_FILE_PATH = 'CON' if platform.system() == 'Windows' else '/dev/stdout'
+
+    def _correct_local_filename(self, filename: str):


This method does not correct a local filename, so its name is confusing. It should be renamed in such a way that the reader can figure out what it does without reading the code to understand what it does.

ppolewicz · 2023-10-19T02:43:14Z

test/unit/test_console_tool.py

+        r_end, w_end = os.pipe()
+
+        # Save the current stdout file descriptor for later restoration
+        stdout_fd = os.dup(1)


should use something like STDOUT_DESCRIPTOR = sys.stdout.fileno() instead of a hardcoded 1 here

ppolewicz · 2023-10-19T02:43:23Z

test/unit/test_console_tool.py

@@ -1210,6 +1210,38 @@ def _test_download_threads(self, download_by, num_threads):
            self._run_command(command)
            self.assertEqual(b'hello world', self._read_file(local_download))

+    def test_download_to_non_seekable_file(self):


you have added new logic in _correct_local_filename, but you haven't added any unit tests for it, while the integration tests for this new feature are insufficient.

mjurbanski-reef reviewed Oct 17, 2023

View reviewed changes

b2/console_tool.py Outdated Show resolved Hide resolved

mjurbanski-reef suggested changes Oct 17, 2023

View reviewed changes

CHANGELOG.md Outdated

Comment on lines 9 to 11

### Fixed

* Fix to support '-' as stdout alias

Copy link

mjurbanski-reef Oct 17, 2023

Choose a reason for hiding this comment

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

this is new feature and not a fix

ppolewicz reviewed Oct 17, 2023

View reviewed changes

athakur-reef force-pushed the atul/b2-70-seeking-stdout branch from 14bd816 to 900957d Compare October 18, 2023 06:06

athakur-reef added 6 commits October 18, 2023 14:16

Use safe_save_to function to check the seekability when download files

79dd92f

Add test for download file to non-seekable file

eddd168

Resolve conflicts for rebase

209c451

Resolve conflicts - rebase and fixed the lint error

71f5c71

Fix the logic of download file and Updated CHANGELOG

b79453d

Fix the stdout_file_path as const of CLASS

Remove Blank Line at CHANGELOG

3046cc7

athakur-reef force-pushed the atul/b2-70-seeking-stdout branch from 7ffef03 to a4bbc47 Compare October 18, 2023 10:34

Fix test code to support windows stdout

09053e3

athakur-reef force-pushed the atul/b2-70-seeking-stdout branch from a15c2b3 to 09053e3 Compare October 18, 2023 13:42

athakur-reef requested review from ppolewicz and mjurbanski-reef October 18, 2023 13:43

athakur-reef changed the title ~~Use "safe_save_to" method to check the seekability when download files~~ Reinforce the "save_to" method to checkout the seekability when download files Oct 18, 2023

athakur-reef changed the title ~~Reinforce the "save_to" method to checkout the seekability when download files~~ Support '-' as STDOUT alias when download files Oct 18, 2023

ppolewicz reviewed Oct 19, 2023

View reviewed changes

mjurbanski-reef closed this Nov 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support '-' as STDOUT alias when download files #211

Support '-' as STDOUT alias when download files #211

athakur-reef commented Oct 17, 2023

mjurbanski-reef commented Oct 17, 2023

athakur-reef commented Oct 17, 2023

athakur-reef commented Oct 17, 2023 •

edited

Loading

mjurbanski-reef left a comment

mjurbanski-reef Oct 17, 2023

athakur-reef commented Oct 17, 2023

ppolewicz left a comment

ppolewicz Oct 17, 2023

ppolewicz Oct 17, 2023

ppolewicz Oct 17, 2023

ppolewicz Oct 17, 2023

ppolewicz Oct 19, 2023

ppolewicz Oct 19, 2023

ppolewicz Oct 19, 2023

ppolewicz Oct 19, 2023

ppolewicz Oct 19, 2023

ppolewicz Oct 19, 2023

		@@ -6,6 +6,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

		## [Unreleased]

		### Added

	def get_download_info_from_local_filename(self, filename: str):
	def _get_download_info_from_local_filename(self, filename: str):

Support '-' as STDOUT alias when download files #211

Support '-' as STDOUT alias when download files #211

Conversation

athakur-reef commented Oct 17, 2023

mjurbanski-reef commented Oct 17, 2023

athakur-reef commented Oct 17, 2023

athakur-reef commented Oct 17, 2023 • edited Loading

mjurbanski-reef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

athakur-reef commented Oct 17, 2023

ppolewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

athakur-reef commented Oct 17, 2023 •

edited

Loading