Add JSON output for select bartib commands#26
Add JSON output for select bartib commands#26MatthiasvB wants to merge 4 commits intonikolassv:masterfrom
Conversation
There was a problem hiding this comment.
I happend to come across the PR and I like the idea!
(I recently wrote a script to select recent activities which would probably be easier to do with that (if I find the time I'll try and add that functionality natively to bartib. Love the project ❤️))
I skimmed the PR and had some thoughts... Do with them what you want 😅
Edit: Link to the mentioned script if anyone needs it
| .short("o") | ||
| .long("output") |
There was a problem hiding this comment.
In my experience "output" is more frequently used for where the outputted data should go.
I would suggest using --format as the long form and maybe -F as the shorthand.
Or make it a boolean flag and just do --json/-j (seen in hyprctl from hyprland).
I would personally prefer the latter (if more formats should come up in the future it could still be changed. Although I would say that if it is likely that there should be more formats supported, the concrete input is probably better as it would be less of a breaking change).
| println!("\n{}", report); | ||
| match format { | ||
| Format::SHELL => println!("{}", report), | ||
| Format::JSON => println!("{}", serde_json::to_string(&report).unwrap()), |
There was a problem hiding this comment.
| Format::JSON => println!("{}", serde_json::to_string(&report).unwrap()), | |
| Format::JSON => println!("{}", serde_json::to_string(&report).unwrap_or_else(|err| { | |
| println!("Could not parse report.\nSerde reported:\n{}", err); | |
| std::process::exit(1); | |
| })), |
I would suggest using something like the above to handle the error case.
That prevents a panic and gives more information as to what happened. (Disclaimer: I have not yet tested the code above.)
|
Thanks again for your work and effort to implement a JSON output format for bartib! It makes me really happy to see people use bartib and help its further development it. Unfortunately, right now, I do not think it would serve the project well to merge the PR. The problem is in my opinion not with your code but with bartibs really simple architecture. In bartib there is today no real separation between the output, the commands and the handling of data. Such a separation would be crucial if bartib should support several output formats. Of course we could implement an ad hoc solution to generate JSON output, but if we were to support other formats like csv, markdown or even html we would need more and more ad hoc solutions. This would quickly lead to some messy code. I hope you understand that for this reason I do not feel comfortable to merge the PR right now. Unfortunately, I do not have the time at this moment to make the necessary adjustments to bartib which would make me comfortable to support different output formats. If I do find the time some day I would be very happy to implement such a feature! |
4 participants
For a (more or less) personal project, I added JSON output to the most essential bartib commands.
I'm not a Rust pro, so I'm very open to and thankful for suggestions for improvements.