-
Notifications
You must be signed in to change notification settings - Fork 273
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
Implements #26: Add info command #55
Conversation
Yay! Will look into this, thanks 🌟 |
} | ||
} | ||
|
||
private struct AppInfoFormatter { |
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.
Could you pull this out into a separate swift class? I just made an enhancement request about release notes, and I think I can implement it using this class by adding a formatUpdateNotes
function of sorts. Then just throw that into the outdated command.
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.
@Aghassi the thing is that I'm still not sure what kind of metadata should be displayed here (e.g. what about an app description, ratings, etc). Maybe it'd be even better to let the user decide what they want. This is basically the only reason why this PR is not merged yet.
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.
In that case, maybe we add a flag? Something like --simple
, which would be the default and give some basic stuff (description, rating, size). Then, another flag like --detailed
that gives more (author, description, rating, size, minimum OS etc.). Finally, there could be a "custom" flag where the user would have to pass in specific fields? That one seems a tad tedious and not so useful though. Just brainstorming a bit, none of these are right or wrong.
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.
maybe a config file to explicitly supply a list of metadata you want?
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.
@garnett Any update on this?
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.
My (admittedly unsolicited) two cents:
Default to simple and flag --verbose
to show all available fields. I would guess anyone truly needing a custom display could grep
the output.
Of course you can do more, but releasing something simple is better than releasing nothing due to indecision!
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 a great discussion, but for now, I've merged this PR as it stands. @Aghassi if you'd still like AppInfoFormatter
to be split out can you open a new issue or PR?
Thanks @garnett! This is a great addition, so I'm merging it as-is. More fields can be added as necessary. |
Implements #26
I was a bit sloppy and haven't added all proposed fields, only stuff I've found personally useful: