Introduce a JSON output table mechanic #248

cas · 2023-01-11T02:54:13Z

cas commented

2023-01-11 02:54:13 +00:00

Create JSONTable as a proxy/extension to tablewriter which can also output JSON.
Implement machine readable output for server list and recipe list

- Create JSONTable as a proxy/extension to tablewriter which can also output JSON. - Implement machine readable output for `server list` and `recipe list`

cas added 1 commit 2023-01-11 02:54:19 +00:00

Introduce a JSON output table mechanic

continuous-integration/drone/push Build is failing

Details

continuous-integration/drone/pr Build is failing

Details

91a2f30136

- Create JSONTable as a proxy/extension to tablewriter which can also output JSON.
- Implement machine readable output for `server list` and `recipe list`

cas commented

2023-01-11 03:04:11 +00:00

fixes #370

decentral1se reviewed 2023-01-11 10:13:43 +00:00

decentral1se left a comment

Looks pretty nice!

There seems to be several hacks here (wouldn't be the first in this codebase 🙃), do you expect they'll come back to bite us @cas? Do you have some concerns that it might break in certain ways?

Maybe we could write some tests for this? We have few tests in the codebase but that is because it normally things rely on this hard-to-test CLI > Logic > Output flow.

But here, you can test your implementation of the JsonTable directly? It might be easier. We did that with tagcmp (see the repo for test examples) and it really paid off, there have been very very few bugs and annoyances for people using abra coming from tagcmp because we ironed out a lot first.

Thanks for picking this up!

Looks pretty nice! There seems to be several hacks here (wouldn't be the first in this codebase 🙃), do you expect they'll come back to bite us @cas? Do you have some concerns that it might break in certain ways? Maybe we could write some tests for this? We have few tests in the codebase but that is because it normally things rely on this hard-to-test CLI > Logic > Output flow. But here, you can test your implementation of the JsonTable directly? It might be easier. We did that with [tagcmp](https://git.coopcloud.tech/coop-cloud/tagcmp) (see the repo for test examples) and it really paid off, there have been very very few bugs and annoyances for people using `abra` coming from `tagcmp` because we ironed out a lot first. Thanks for picking this up!

cas commented

2023-01-11 21:31:15 +00:00

Looks pretty nice!

There seems to be several hacks here (wouldn't be the first in this codebase 🙃), do you expect they'll come back to bite us @cas? Do you have some concerns that it might break in certain ways?

I don't feel too concerned about it breaking. The only thing that worries me is that it has a slightly brittle way of producing JSON and it proxies another type which may change under us (I'm not too concerned about the proxy part since the use in abra of tablewriter is really specific and we'll be able to catch when that changes easily).

Maybe we could write some tests for this? We have few tests in the codebase but that is because it normally things rely on this hard-to-test CLI > Logic > Output flow.

But here, you can test your implementation of the JsonTable directly? It might be easier. We did that with tagcmp (see the repo for test examples) and it really paid off, there have been very very few bugs and annoyances for people using abra coming from tagcmp because we ironed out a lot first.

We could test the JSONTable type, I expect the best tests would just be throwing things at and a) testing if the output is valid JSON (parsable) and b) that the output is identical to a raw tablewriter.Table.

Thanks for picking this up!

No worries, it's great to do this work!

> Looks pretty nice! > > > There seems to be several hacks here (wouldn't be the first in this codebase 🙃), do you expect they'll come back to bite us @cas? Do you have some concerns that it might break in certain ways? I don't feel too concerned about it breaking. The only thing that worries me is that it has a slightly brittle way of producing JSON and it proxies another type which may change under us (I'm not *too* concerned about the proxy part since the use in `abra` of tablewriter is really specific and we'll be able to catch when that changes easily). > Maybe we could write some tests for this? We have few tests in the codebase but that is because it normally things rely on this hard-to-test CLI > Logic > Output flow. > > But here, you can test your implementation of the JsonTable directly? It might be easier. We did that with [tagcmp](https://git.coopcloud.tech/coop-cloud/tagcmp) (see the repo for test examples) and it really paid off, there have been very very few bugs and annoyances for people using `abra` coming from `tagcmp` because we ironed out a lot first. We could test the JSONTable type, I expect the best tests would just be throwing things at and a) testing if the output is valid JSON (parsable) and b) that the output is identical to a raw tablewriter.Table. > Thanks for picking this up! No worries, it's great to do this work!

🎉 1

cas added 1 commit 2023-01-12 04:29:09 +00:00

Add tests to jsontable.

continuous-integration/drone/pr Build is failing

Details

continuous-integration/drone/push Build is failing

Details

c66b26075a

- Test major functionality of jsontable
- Fix bug discovered in testing.

cas commented

2023-01-12 04:30:05 +00:00

I added tests and in writing the tests discovered a bug in jsontable. Testing Works™.

More tests could be added but I think this should get us a long way there.

I added tests and in writing the tests discovered a bug in jsontable. Testing Works™. More tests could be added but I think this should get us a long way there.

decentral1se approved these changes 2023-01-12 20:18:23 +00:00

decentral1se left a comment

Solid! LGTM then, merge away 🚀

cas merged commit 514ed766e9 into main

2023-01-12 21:15:22 +00:00

cas deleted branch cr_machinereadable_2

2023-01-12 21:15:27 +00:00

decentral1se referenced this pull request from toolshed/organising

2023-01-12 21:28:30 +00:00

Machine-readable output for `abra recipe ls` #370

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: toolshed/abra#248