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

csjones · 2020-08-17T07:03:22Z

🤔 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:

Adds the optional flag -v/--verbose to display more in terminal.
Changed output of all commands to be parsable by default
Removed .prettyPrinted and created Add a flag to prettify output #221

⚠️ 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 · 2020-08-19T03:43:20Z

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 · 2020-08-19T04:06:45Z

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 · 2020-08-20T05:35:43Z

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 · 2020-08-20T06:00:54Z

@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 · 2020-08-20T10:21:19Z

@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 · 2020-08-21T01:52:43Z

@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:

Change --quiet to be --verbose
--verbose defaults to false except when --table is used
Remove use of .prettyPrinted
Create GitHub issue for --pretty feature flag

adamjcampbell · 2020-08-21T02:06:53Z

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

adamjcampbell · 2020-08-25T03:45:46Z

Sources/AppStoreConnectCLI/Commands/Certificates/ListCertificatesCommand.swift

@@ -55,7 +55,10 @@ struct ListCertificatesCommand: CommonParsableCommand {

                let file = try certificateProcessor.write($0)

-                print("📥 Certificate '\($0.name ?? "")' downloaded to: \(file.path)")
+                // Command output is parsable by default. Only print if user is enabling verbosity or output is a `.table`
+                if common.verbose || common.outputFormat == .table {


We seem to have to use common.verbose || common.outputFormat == .table quite a bit here.
What do you think about making verbose be Bool? (defaulting to nil) and then we can add a computed var either a Bool or enum to check.

Thinking along these lines:

// CommonParsableCommand.swift enum PrintLevel { case quiet case verbose } // CommonOptions var printLevel: PrintLevel { switch verbose { case true: return .verbose case false: return .quiet case nil: return common.outputFormat == .table ? .verbose : .quiet } } // Usage if common.printLevel == .verbose { ... }

Overall, this is a good suggestion and I'll make these changes with small tweaks. I'll leave verbose as a Bool and I'll resolve the common.outputFormat == .table ternary in the switch statement itself. Should look something like this:

// CommonOptions var printLevel: PrintLevel { switch (verbose, outputFormat == .table) { case (true, _), (_, true): return .verbose case (false, false): return .quiet } }

Sounds good, thanks @csjones

adamjcampbell

@csjones This is looking ✅, just needs the swiftlint warnings attended to

csjones added 3 commits August 16, 2020 17:00

Adding --quiet flag

59835fa

Better help information

e3739f0

Merge branch 'master' into csjones-quiet

84c8f97

csjones mentioned this pull request Aug 23, 2020

Add a flag to prettify output #221

Open

Refactored quiet into verbose

f3b5c87

csjones changed the title ~~Adding a quiet flag for displaying less messages in terminal~~ Adding a verbose flag for displaying more messages in terminal Aug 23, 2020

adamjcampbell reviewed Aug 25, 2020

View reviewed changes

csjones added 2 commits August 25, 2020 21:51

Created PrintLevel, cleaned up duplicative logic

d6b5bea

Cleanup

d53dae7

adamjcampbell approved these changes Aug 26, 2020

View reviewed changes

Fixed swiftlint issues

cdb7c32

adamjcampbell merged commit c176752 into ittybittyapps:master Aug 26, 2020

csjones deleted the csjones-quiet branch January 28, 2023 23:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

csjones commented Aug 17, 2020 •

edited

Loading

adamjcampbell commented Aug 19, 2020 •

edited

Loading

orj commented Aug 19, 2020

csjones commented Aug 20, 2020 •

edited

Loading

adamjcampbell commented Aug 20, 2020

orj commented Aug 20, 2020

csjones commented Aug 21, 2020 •

edited

Loading

adamjcampbell commented Aug 21, 2020

adamjcampbell Aug 25, 2020

csjones Aug 26, 2020

adamjcampbell Aug 26, 2020

adamjcampbell left a comment

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

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

Conversation

csjones commented Aug 17, 2020 • edited Loading

🤔 Objective and Motivation

📝 Summary of Changes

⚠️ Items of Note

🧐🗒 Reviewer Notes

💁 Example

🔨 How To Test

adamjcampbell commented Aug 19, 2020 • edited Loading

orj commented Aug 19, 2020

csjones commented Aug 20, 2020 • edited Loading

adamjcampbell commented Aug 20, 2020

orj commented Aug 20, 2020

csjones commented Aug 21, 2020 • edited Loading

adamjcampbell commented Aug 21, 2020

adamjcampbell Aug 25, 2020

Choose a reason for hiding this comment

csjones Aug 26, 2020

Choose a reason for hiding this comment

adamjcampbell Aug 26, 2020

Choose a reason for hiding this comment

adamjcampbell left a comment

Choose a reason for hiding this comment

csjones commented Aug 17, 2020 •

edited

Loading

adamjcampbell commented Aug 19, 2020 •

edited

Loading

csjones commented Aug 20, 2020 •

edited

Loading

csjones commented Aug 21, 2020 •

edited

Loading