ittybittyapps / appstoreconnect-cli

An easy to use command-line tool for interacting with the Apple AppStore Connect API
MIT License
172 stars 17 forks source link

Adding a verbose flag for displaying more messages in terminal #220

Closed csjones closed 3 years ago

csjones commented 3 years ago

πŸ€” Objective and Motivation

The objective of these changes are to purpose adding an optional flag -v/--verbose to display more output in terminal. My motivation for this flag is to remove overhead when using asc with ansible. I'm using the following command in my ansible playbook:

asc profiles list --json --filter-name "App Store {{ app_name }}" --download-path .

...and I get the following output:

"stdout": "πŸ“₯ Profile 'App Store Test001010002' downloaded to: ./aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa.mobileprovision\n[{\"createdDate\":619333688,\"expirationDate\":620178602,\"name\":\"App Store Test001010002\",\"platform\":\"IOS\",\"profileContent\":\"MdMIIdqAAYJKoZIhvcNAQQcCoIIdmTCCHZCUCAQExCzACJB........

The "stdout" is now malformed json and requires additional parsing to work correctly by filtering out the "πŸ“₯ Profile 'App Store Test001010002' downloaded to: ./aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa.mobileprovision\n". With the --verbose flag added, the command now looks like this:

asc profiles list --verbose --json --filter-name "App Store {{ app_name }}" --download-path .

...and I get the following output:

"stdout": "πŸ“₯ Profile 'App Store Test001010002' downloaded to: ./aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa.mobileprovision\n[{\"createdDate\":619333688,\"expirationDate\":620178602,\"name\":\"App Store Test001010002\",\"platform\":\"IOS\",\"profileContent\":\"MdMIIdqAAYJKoZIhvcNAQQcCoIIdmTCCHZCUCAQExCzACJB........

...and the default command and output looks like this:

asc profiles list --json --filter-name "App Store {{ app_name }}" --download-path .

...and the json returned to stdout is no longer malformed so now I get the following output:

"stdout": "[{\"createdDate\":619333688,\"expirationDate\":620178602,\"name\":\"App Store Test001010002\",\"platform\":\"IOS\",\"profileContent\":\"MdMIIdqAAYJKoZIhvcNAQQcCoIIdmTCCHZCUCAQExCzACJB........

πŸ“ Summary of Changes

Changes proposed in this pull request:

⚠️ Items of Note

Document anything here that you think the reviewer(s) of this PR may need to know, or would be of specific interest.

The --verbose only changes the output of a CommonParsableCommand that has not been commented with a TODO.

πŸ§πŸ—’ Reviewer Notes

πŸ’ Example

swift run asc profiles list --verbose --json --filter-name "Cool App" --download-path ~/Documents/Cool\ App

πŸ”¨ How To Test

Run these command and notice the messaging in terminal:

swift run asc profiles list --json --filter-name "Cool App" --download-path ~/Documents/Cool\ App
swift run asc users sync ~/Documents/fake.json --dry-run
swift run asc certificates list
swift run asc certificates read "M109UYTGSF007" --certificate-output distribution.cer
swift run asc certificates revoke "M109UYTGSF007"

Run the same commands with -v/--verbose added and notice more messaging in terminal:

swift run asc profiles list --verbose --json --filter-name "Cool App" --download-path ~/Documents/Cool\ App
swift run asc users sync -v ~/Documents/fake.json --dry-run
swift run asc certificates list -v
swift run asc certificates read --verbose "M109UYTGSF007" --certificate-output distribution.cer
swift run asc certificates revoke --verbose "M109UYTGSF007"
adamjcampbell commented 3 years ago

Hey @csjones, thank you for your contribution. You've prompted a discussion on our end with this PR about whether quiet, output only commands should be the default response. That is, whether or not this should be implemented as only logging these Certificate downloaded Certificate revoked messages when a --verbose flag is passed.

This would then need to be rolled out across other commands so that the output of all commands is parsable by default. Did you have any other thoughts on the matter for your use case?

orj commented 3 years ago

I agree with @adamjcampbell here. I think the stuff like πŸ“₯ Profile 'X' downloaded to: ... should only be printed when a --verbose flag was in effect.

πŸ€” One thing to consider is perhaps when using --json, --yaml, and --csv, --verbose could be false but with --table it could be true? Given that the computer parsable formats are more likely to be used in pipes than the table output which is designed for interactive use. Thoughts?

csjones commented 3 years ago

Hey @adamjcampbell @orj, I'm in favor of these changes. I think machine parsable by default and a --verbose option instead of --quiet makes a lot of sense, especially for my use case. Also makes sense to default --verbose to true when using the human parseable --table option.

I would propose one more additional change. When using --json (or any machine parsable format) to minimize the output by default instead of pretty printing the output. Currently, --json is using a prettify option and adds additional whitespace and newlines which needs to be filtered out.

adamjcampbell commented 3 years ago

@csjones sounds good to me. Are you happy to make those changes for this command / this PR?

In regards to the pretty formatting of json I would think filtering out the white space would be optional. It shouldn't affect the ability to parse the JSON. However we could have it as an option or the default when not in verbose mode.

What are your thoughts on the matter?

orj commented 3 years ago

@adamjcampbell Perhaps we can have a --pretty option for prettifying the json. Mostly I think you would be piping json output into a tool like jqand jq can pretty it for you.

csjones commented 3 years ago

@adamjcampbell Sure! I'll make the changes and update the PR. For pretty printing output, I'm leaning towards removing .prettyPrinted since --pretty currently does not exist and I prefer @orj's idea of using jq if needed because my use case is aimed towards better accommodating machine parsable output. Additionally, I could also create a new GitHub issue to add a --pretty option and see if any other asc users would prefer to have this feature flag added.

Does this list of changes look good?

Summary of changes:

adamjcampbell commented 3 years ago

@csjones sounds like a plan! Thanks for all your efforts on this.