Skip to content

Conversation

@marcelm
Copy link
Collaborator

@marcelm marcelm commented Jan 15, 2024

No description provided.

Copy link
Collaborator

@rhpvorderman rhpvorderman left a comment

Choose a reason for hiding this comment

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

Nice cleanup! I have a few small suggestions.

README.rst Outdated

* `python-isal <https://github.com/pycompression/python-isal>`_.
Supports multiple threads and compression levels up to 3.
* zlib-ng
Copy link
Collaborator

Choose a reason for hiding this comment

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

* `python-zlib-ng <https://github.com/pycompression/python-zlib-ng>`_. 
  Supports multiple threads and compression levels up to 9.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Whoops, I wrote this while our broadband was updated and had no internet access. I had intended meant to add the link later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

BTW, if you make these comments on the 'Files changed' tab, then you can use the 'Suggestions' button and I can just click to accept them.

See point 6 at https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/commenting-on-a-pull-request#adding-comments-to-a-pull-request

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, that is good to know. Thanks!

Supports multiple threads and compression levels up to 3.
* zlib-ng
* `pigz <https://zlib.net/pigz/>`_ (a parallel version of ``gzip``)

Copy link
Collaborator

Choose a reason for hiding this comment

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

* `gzip <https://www.gnu.org/software/gzip/>`_. 

README.rst Outdated
Comment on lines 112 to 121
If multi-threaded compression or decompression is available,
this parameter can be used to override the number of threads
used. It is ignored otherwise.

Set threads to 0 to force opening the file without using a subprocess.
For some compression levels,
compressed files are by default read or written
using a pipe to a subprocess running an external tool such as
``pbzip2`` or ``xz``.
With *threads* set to 0, a normal function call is used instead.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The above is not entirely correct as not always a subprocess is used. Below is my attempt at the paragraph. It is a hard thing to properly explain in a few short sentences so please do not take this as a literal suggestion:

Sets the number of threads spawned to open a compressed file. May be ignored if the backend does not support threads.

By default xopen tries to offload the (de)compression work load to other threads to free up the main Python thread for the application. This can either be done by using a subprocess to an external application or using a library that supports threads.

Set threads to 0 to force xopen to use only the main Python thread.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I changed it to a variant of your suggestion. I agree it’s hard to explain. Maybe an extra section is needed in the documentation somewhere (later). As this paragraph is just the explanation of the parameter, I think it should be relatively short.

@rhpvorderman
Copy link
Collaborator

Looks good to me!

@rhpvorderman rhpvorderman merged commit 9afe371 into main Jan 17, 2024
@rhpvorderman rhpvorderman deleted the readme2 branch January 17, 2024 08:18
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.

None yet

3 participants