add support for --list --json #74

Merged
witten merged 5 commits from :list-json into master 2018-07-28 21:21:39 +00:00
Contributor

as discussed, here's a first try at supporting json output from borgmatic for the borg list command.

Outputed json is wrapped in an array to handle multi-repository cases.

Note I had to use tempfile.TemporaryFile() since suprocess.check_call() does not support io.StringIO() for its stdout parameter.

as [discussed](https://projects.torsion.org/witten/borgmatic/issues/61#issuecomment-620), here's a first try at supporting json output from borgmatic for the `borg list` command. Outputed json is wrapped in an array to handle multi-repository cases. Note I had to use `tempfile.TemporaryFile()` since `suprocess.check_call()` does not support `io.StringIO()` for its `stdout` parameter.
Owner

Finally got a chance to look at this. Thanks for making this enhancement! Here's some feedback:

  • Good idea to wrap the JSON in a list, given that there are potentially multiple repositories.
  • Minor nit: Convention here is alphabetical ordering on the imports.
  • I think that instead of using subprocess.check_call() with a temporary file, you should just be able to switch to subprocess.check_output() and get the output you're looking for as a return value of that call. Then you won't need that temp file.
  • It'd be good to update the doc string on list_archives() to telegraph the contract change: That sometimes it returns an output value and sometimes it doesn't, based on the value of the json parameter.
  • Or, have you considered just making list_archives() unconditionally return the output value (regardless of JSON or not), and make the caller responsible for dealing with it? That might simplify the contract, since the caller is having to deal with the output anyway.
  • Thanks for writing automated tests! It's unfortunate that the existing run_coniguration() function is so difficult to test, as it would be nice to have some coverage around the outer JSON list assembly and output. I'll have to think about how to make this function more testable.. If you have any ideas, let me know.
Finally got a chance to look at this. Thanks for making this enhancement! Here's some feedback: * Good idea to wrap the JSON in a list, given that there are potentially multiple repositories. * Minor nit: Convention here is alphabetical ordering on the imports. * I think that instead of using `subprocess.check_call()` with a temporary file, you *should* just be able to switch to `subprocess.check_output()` and get the output you're looking for as a return value of that call. Then you won't need that temp file. * It'd be good to update the doc string on `list_archives()` to telegraph the contract change: That sometimes it returns an output value and sometimes it doesn't, based on the value of the `json` parameter. * Or, have you considered just making `list_archives()` unconditionally return the output value (regardless of JSON or not), and make the caller responsible for dealing with it? That might simplify the contract, since the caller is having to deal with the output anyway. * Thanks for writing automated tests! It's unfortunate that the existing `run_coniguration()` function is so difficult to test, as it would be nice to have some coverage around the outer JSON list assembly and output. I'll have to think about how to make this function more testable.. If you have any ideas, let me know.
Owner

Also: Reminder to update the AUTHORS file if you like!

Also: Reminder to update the AUTHORS file if you like!
Author
Contributor

MR updated based on your feedback.

I also gave a try at testing the JSON list assembly bit in run_configuration().

And it appears I'm already in the AUTHORS list ;)

MR updated based on your feedback. I also gave a try at testing the JSON list assembly bit in `run_configuration()`. And it appears I'm already in the AUTHORS list ;)
Owner

Awesome, thanks for incorporating feedback and doing the refactoring to support test coverage. Looks good.. I'll merge this now. There are a few (totally undocumented) coding convention issues around whitespace, but I can fix those after the merge.

Awesome, thanks for incorporating feedback and doing the refactoring to support test coverage. Looks good.. I'll merge this now. There are a few (totally undocumented) coding convention issues around whitespace, but I can fix those after the merge.
witten closed this pull request 2018-07-28 21:21:39 +00:00
Owner

Oh, and about the AUTHORS file: It just mentions you in regards to one particular feature, so feel free to take credit for additional features there if you like. (Or just generalize the description if that sounds too high-maintenance.) I just want to make sure credit is given appropriately!

Oh, and about the AUTHORS file: It just mentions you in regards to one particular feature, so feel free to take credit for additional features there if you like. (Or just generalize the description if that sounds too high-maintenance.) I just want to make sure credit is given appropriately!
thomasleveil deleted branch list-json 2018-07-28 22:35:39 +00:00
Owner

Released in borgmatic 1.2.1!

Released in borgmatic 1.2.1!
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: borgmatic-collective/borgmatic#74
No description provided.