luizirber commented 4 years ago

Per https://github.com/luizirber/niffler/issues/1#issuecomment-565130377

Missing:

[ ] stdin (as -) support
[ ] docs
[ ] tests

Questions:

[ ] Do we want to do format detection for outputs based on path name? In this case to_path should take Format and Level as Options.
[ ] Bikeshed on function name? from_path is quite common in Rust codebases, but not very happy with to_path...
[ ] also use - in to_path as shortcut for stdout?

/cc @enormandeau

codecov[bot] commented 4 years ago

Codecov Report

Merging #12 into master will decrease coverage by 0.42%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #12      +/-   ##
==========================================
- Coverage   76.06%   75.64%   -0.43%     
==========================================
  Files           2        2              
  Lines         234      234              
  Branches       36       35       -1     
==========================================
- Hits          178      177       -1     
  Misses         24       24              
- Partials       32       33       +1

Impacted Files	Coverage Δ
src/compression.rs	`67.18% <100.00%> (ø)`
src/lib.rs	`85.84% <100.00%> (-0.95%)`	:arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update aa362f1...e30542f. Read the comment docs.

enormandeau commented 4 years ago

I would suggest from_file and to_file since the paths are files, not directories.

natir commented 4 years ago

Hello every one,

I have some trouble with this PR. In fact more precisely on missing or question.

I didn't like from_path, to_path function because the user can open those files alone and pass the stream to get_input and do what this want with the stream. But okay this function are user-friendly I didn't like it but it's required.

About the idea when input_name is - we read stdin I think that haven't any place in a crate. I can accept this behavior for a binary, but if we implement this in a library crate we make this choice for all user of our crate. I strongly disagree with this idea. (Same stuff for idea of use - to write in stdout)

About the detection of output compression with the filename of output, I didn't like this idea. An example, John create a bin they use niffler and I allow users to specify output name if the user make a mistake in filename, test.gizp in place of test.gzip, the output isn't compressed. We can find some alternative of this bug very easily. User can consider this as a bug. If we implement this functionality in niffler we are responsible of this bug, if John implements it in their bin it's John bug. My favourite bugs are the ones I'm not responsible for.

I hope I was clear and not to rude or disrespectful if someone felt attacked by this comment, please excuse me.

natir commented 4 years ago

Up ! :)

luizirber commented 4 years ago

@natir two things happening here: 1) the to_path and from_path functions, withou checking for - as stdin/stdout (that's https://github.com/luizirber/niffler/pull/12/commits/f13103ee17b5712af8d0ff305dd3369b8ce5fdda) 2) I tried to use get_reader in a method in sourmash, and it complained because it was expecting Box<dyn io::Read + 'static>, which is very restrictive. I added lifetimes to all methods taking/returning Box and made the bounds Box<dyn io::Read + 'a>, which solves the problem. That's https://github.com/luizirber/niffler/pull/12/commits/e30542f37a280d8644992e016a2619f33ca2f4d1 and more info in https://users.rust-lang.org/t/box-with-a-trait-object-requires-static-lifetime/35261/2

If you agree, ready for review and merge =]

luizirber / niffler

Add from/to path functions #12

Codecov Report