Closed wfondrie closed 1 year ago
LGTM (the emojis are a nice touch)! Only suggestion I have is to run cargo fmt
.
As far as testing CLI... I'm not sure what the best practice is for doing this. I think adding in a test case to the rust.yml
github workflow would be sufficient - I'm not super concerned about test coverage for stuff like that though (although happy to add it in if you find a solution!)
Thanks for doing this, adding some more CLI options has been on my to-do list for a while, stuffing everything in a JSON file was just the least friction for testing. And sorry for the state of the code in that file, I've been refactoring some of the glue code to make it easier to tweak.
Thanks! I've run cargo fmt
and added a simple test to the Github Action. Just let me know if you want any other changes before merging.
Thanks for doing this, adding some more CLI options has been on my to-do list for a while, stuffing everything in a JSON file was just the least friction for testing. And sorry for the state of the code in that file, I've been refactoring some of the glue code to make it easier to tweak.
It's no problem at all - that is the fun of open-source right? Also, I love having configuration in a JSON file. Overall I've found the code in Sage to be very easy to follow and helpful introduction to Rust 😄
I am not 100% sure if this is the right place to report it BUT I noticed the output directory is not respected for the "results.json" file, which gets generated in the current working directory regardless of the passed out dir
if I knew more than how to write hello world in rust I would gladly try to fix it BUT i think the line is this one ...
https://github.com/lazear/sage/blob/ff860db20774832e3e8551b48a0c91eb35ee0ba2/src/bin/sage.rs#L347
LMK what you think, is this behavior expected/desired?
@jspaezp This is a good point - I'm doing a rework over in the lfq branch (#17) to try and clean up some of the output files, so I am open to suggestions on where to place this file (or if writing to stdout is more appropriate)
@lazear I personally really like the idea of having the output in the output dir. maybe even as a outdir/results.sage.json, in addition to stdout. Nontheless, I feel like either option would be fine. (out dir is ok, stdoud is ok, stdout and out dir is ok, workdir is weird ... to summarize it).
Thanks a lot!
This PR adds some parameters to the CLI, which overide the configuration provided in the param file. The included parameters are:
mzml_paths
database.fasta
output_directory
This makes it easier to construct a default config file and use it for many analyses. Now you can run:
I didn't write tests... mostly because I don't know how to test a Rust CLI yet.
Please note that I just finished reading the Rust book this weekend, so certainly no offense if there's a lot of changes to make. 😆