Flag options for different stat categories #12
Conversation
aaronshim
force-pushed
the
flags
branch
2 times, most recently
from
9b03403 to
1d7b71b
Compare
|
(Also, there are a lot of repeat commits from the fact that I branched this work off of an existing working branch off my fork, with pending changes in other PRs. Once the others go in, I will rebase this to get rid of duplicate commits.) |
|
Actually, totally found the answer to the whole "let's not leave a bunch of null keys in our JSON" story: It's talked about at serde-rs/serde#65. |
|
Finally getting around to this one, too. @aaronshim could you please rebase/merge from my master with the code from #11 that I just merged to clear up the duplicates and resolve the conflicts? |
(cherry picked from commit 41fa73ac17d62dddff1daef075bdef54d8da1e4f)
…r than having them null)
|
Rebased and put in the serde serialization options to skip null. |
|
I've been traveling for a few days, so I'm just now taking a look at this--looks good so far. |
| // We will need to pass this down in the main thread, to the individual printers, | ||
| // and there is no point in passing down the whole Args struct, in the spirit of encapsulation. | ||
| #[derive(Debug)] | ||
| pub struct StatArgs { |
There was a problem hiding this comment.
This feels a little un-DRY. For the sake of encapsulation, it definitely makes sense to pass the stat args around as their own struct, but I'm wondering whether there's a way we can eliminate the fields from Args that are later present in StatArgs. That way, we can either construct a full set of args using composition (which admittedly I haven't tried in Rust), or just see whether you can read into both structs with Docopt::new somehow.
(That, or we can just rename StatArgs to something that would make it conceptually distinct from command line arguments lol.)
| // I wanted to prevent the evaluation of as many of these parameters at this level as possible | ||
| // but it seems that because compression rate depends on both .size() and .compressed_size(), we | ||
| // need both to be passed in to calculate compression_rate if needed. (No way to encode this | ||
| // dependency here without dependent types.) |
| // all of these are going to be calculated, based on certain | ||
| // command-line flags given to this program. | ||
| // | ||
| // TODO: Figure out how to get the JSON serializer to dump fields |
There was a problem hiding this comment.
Are you satisfying the TODO with the #[serde(skip_serializing_if="Option::is_none")]?
| let multi_archive = MultiArchiveJsonWriter::from(file_paths,exclude); | ||
| serde_json::to_string_pretty(&multi_archive).unwrap() | ||
| fn zips_to_json_with_printer<F>(file_paths: &[&str], exclude: &str, stat_args: &StatArgs, printer: F) -> String | ||
| where F: Fn(&MultiArchiveJsonWriter) -> serde_json::Result<String> { |
There was a problem hiding this comment.
Great refactoring using the where clause 💯
There was a problem hiding this comment.
I accidentally hit "start review" instead of just "single comment"--note that there are still comments, too
| @@ -15,10 +15,12 @@ mod flat_writer; | |||
| mod json_writer; | |||
|
|
|||
| use zip_info::WriteZipInfo; | |||
| use zip_info::Args; | |||
| use zip_info::StatArgs; | |||
There was a problem hiding this comment.
We can move all of these on one line using the shortcut described here.
| use zip_info::WriteZipInfo; | ||
| use zip_info::StatArgs; |
|
Other than my bikeshedding, it does what it says on the tin 👍 |
Fixes #1 .
Do we want to think about a further DRY optimization where we don't repeat the code that generates the statistics between the
flat_writerand thejson_writerpaths? Perhaps another struct as an abstraction of thefile -> statscomputation that implements traitsto_flat_writerandto_json_writersince only the representation of the stats computation are different?