-
Notifications
You must be signed in to change notification settings - Fork 56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add short output #1167
Add short output #1167
Conversation
b4d1581
to
42e23c9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an interesting idea, what if we would make it valid yaml so that it would look nice but still be (relatively) easy to parse? How would that look like? Note that I don't think we need a fully yaml parser/struct here (at least initially) to test the idea, just re-structuing the output that its a valid yaml map if distroname that then has keys with type and a list of arches (which is super close to what is written here already). Wdyt?
a14bf79
to
236403a
Compare
I added a colon after the distro, so now technically it's yaml but the arches are no list :-/ |
Fixed spellchecking by changing to `code` as this is accepted and also more valid semantically.
The 'short' output format should provide a better overview when looking at all combinations.
236403a
to
b2b79c1
Compare
Thank you @mvo5 for your hint, it seems possible to have this a valid yaml. The structure does not make too much sense, I guess (to nest |
btw I tested the output with:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you.
Some remarks but non-blocking:
- Should we instead call this
yaml
if we're committed to making it YAML? - In that case, should we 'just' have a proper yaml encoder just like
json
?
I don't particularly like the spellcheck commit showing up in this PR but if it blocks CI otherwise, I guess.
For |
Fwiw, I think this got merged a bit too quickly, I think there are still open questions like how exactly this should look like (and I would have liked to review the new implementation too). There is also the open question of how this relates to https://github.com/osbuild/osbuild-composer/pull/4336/files#diff-792fae8eca7351040416f58a710aa554fe76d91e6f39cfbe122bf85428f34afdR572 - which is similar but slightly different which is now confusing for users of our API. |
Sorry, I thought all your remarks have been addressed :-/ |
Add
short
outputwhich looks like
(this is based on #1166 )