Skip to content

Allow setting preset for LZMACompressorOutputStream#664

Closed
dwalluck wants to merge 1 commit intoapache:masterfrom
dwalluck:lzma-preset
Closed

Allow setting preset for LZMACompressorOutputStream#664
dwalluck wants to merge 1 commit intoapache:masterfrom
dwalluck:lzma-preset

Conversation

@dwalluck
Copy link
Copy Markdown

No description provided.

Copy link
Copy Markdown
Member

@garydgregory garydgregory left a comment

Choose a reason for hiding this comment

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

Hi @dwalluck

Thank for your PR.

Let's not go start going down this road or we'll end up with asks for variations of:

    public LZMA2Options(int dictSize, int lc, int lp, int pb, int mode,
                        int niceLen, int mf, int depthLimit)

Instead, please try to use this pattern in git master:

        try (LZMACompressorOutputStream out = LZMACompressorOutputStream.builder()
                .setPath(outPath)
                .setLzma2Options(options)
                .get()) {
            out.writeUtf8(data);
        }

I can see keeping the "normal" one-arg constructor around and using the builder for anything that requires a non-default configuration.

@dwalluck
Copy link
Copy Markdown
Author

@garydgregory I understand. I only thought to add it for parity with XZ which already has this constructor.

@dwalluck
Copy link
Copy Markdown
Author

Wait, maybe I don't understand. You want to create a Builder just for the options? But, if we add a second arg of type LZMA2Options, that also would take care of that, no?

@ppkarwasz
Copy link
Copy Markdown
Member

There is already a Builder.setLzma2Options(options) method, so there is no need to add a new constructor.

@dwalluck
Copy link
Copy Markdown
Author

Ah, it was just added that same day? That was fast! Thanks!

However, if there were a builder for the options themselves, then the same pattern could be shared with xz, too. Otherwise, it seems xz needs its own builder, too, right?

Similarly, I think zstd needs a way to get at all of the options that are available via the ZstdOutputStream::set methods. In particular, I wanted to get at ZstdOutputStream::setWorkers to enable parallel compression. Then the ZstdCompressorOutputStream(final OutputStream outStream, final int level, ...) constructors could also be deprecated the way you did with LZMA.

@ppkarwasz
Copy link
Copy Markdown
Member

Ah, it was just added that same day? That was fast! Thanks!

Right, I didn't notice, this was pushed directly to master in 55ecb7f.
@garydgregory, could we use PRs for this kind of changes? A direct push to master kind of cuts short discussions on PRs likes this one. If you create a new PR, the discussion can continue in the new PR.

@garydgregory
Copy link
Copy Markdown
Member

garydgregory commented Apr 20, 2025

Ah, it was just added that same day? That was fast! Thanks!

However, if there were a builder for the options themselves, then the same pattern could be shared with xz, too. Otherwise, it seems xz needs its own builder, too, right?

Similarly, I think zstd needs a way to get at all of the options that are available via the ZstdOutputStream::set methods. In particular, I wanted to get at ZstdOutputStream::setWorkers to enable parallel compression. Then the ZstdCompressorOutputStream(final OutputStream outStream, final int level, ...) constructors could also be deprecated the way you did with LZMA.

Hello @dwalluck

Please see PR #666 to add a builder to ZstdCompressorOutputStream.

TY,
Gary

@garydgregory
Copy link
Copy Markdown
Member

Closing: The feature is in git master through a builder.

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