haskell-hvr / lzma

Haskell bindings to liblzma
https://hackage.haskell.org/package/lzma
BSD 3-Clause "New" or "Revised" License
18 stars 11 forks source link

`DecompressParams` should be modifiable? #5

Open taktoa opened 7 years ago

taktoa commented 7 years ago

There's currently no function exported fromCodec.Compression.Lzma that allows defaultDecompressParams to be customized. It seems to me that, this being the case, either the constructor/fields of DecompressParams should be exported, or, if this was the intended semantics, there is no point in exporting decompressWith or DecompressParams at all. Since LibLzma is not an exposed module there is simply no way to create a value of type DecompressParams other than the default.

The same argument also applies to CompressParams. The current interface is about as useful as a module with the following exports:

module Codec.Compression.Lzma
  ( compress, decompress
  , CompressStream (..), compressIO, compressST
  , DecompressStream (..), decompressIO, decompressST
  , LzmaRet (..)
  ) where

So, IMO, the interface should be simplified to the above (if this is what is intended), or the constructor/fields for CompressParams/DecompressParams should be exposed.

BTW, unless I'm ignorant of some other factor, decompressAutoDecoder should probably default to True.

hvr commented 7 years ago

@taktoa The Haddock documentation doesn't make it obvious, but the functions under http://hackage.haskell.org/package/lzma-0.0.0.3/docs/Codec-Compression-Lzma.html#g:6 are actually the field accessors. So you can in fact use record-update syntax such as e.g.

defaultDecompressParams { decompressAutoDecoder = True, decompressMemLimit = 16777216 }

Btw, the reason for not exposing the constructor is so that we don't have to perform major version bumps each time a new field is added to DecompressParams or CompressParams; so this way we only need to perform a minor version increment, as a new field is purely a backward compatible API addition.

Why do you believe that the deprecated .lzma format should be auto-detected by default?

taktoa commented 7 years ago

Ah, my bad. Is there a way to convince Haddock to output more sane documentation for this?

Regarding the .lzma format: on several occasions I've compressed files with the lzma command line utility (resulting in a file in the deprecated format) and then tried to decompress them with this library only to be confronted by a relatively cryptic error. I think most users expect a library called lzma to be able to decompress files produced by lzma by default. If you think having support for a deprecated format is bad design, then at least add something to the documentation to warn naive users.

hvr commented 7 years ago

Good point. I'll try to add some visible hints in the documentation for both, the auto-decoder-default as well as the Haddock-rendering issue of showing field updaters as ordinary functions.

Fwiw, I went with what the default was with the liblzma C library, whose default entrypoints are designed to accept .xz only by default,

        LZMA_FORMAT_ERROR       = 7,
                /**<
                 * \brief       File format not recognized
                 *
                 * The decoder did not recognize the input as supported file
                 * format. This error can occur, for example, when trying to
                 * decode .lzma format file with lzma_stream_decoder,
                 * because lzma_stream_decoder accepts only the .xz format.
                 */