Skip to content
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 format flag to listen cmd #615

Merged
merged 4 commits into from
Mar 18, 2021

Conversation

gracegoo-stripe
Copy link
Contributor

Reviewers

r? @tomer-stripe
cc @stripe/developer-products

Summary

The --format flag is a string that currently only takes in JSON as a valid input option, similar to the log tailing command.
It's intended to replace the --print-json for the listen cmd. The difference in behavior between --format JSON and --print-json is that the new flag will stripe out all formatting in the payload.

While working on the event streaming feature in VSCode, the stream was getting partial json because the json was being pretty-printed. This PR updates the listen cmd to behave the same way as the log tail command.

Testing

With the deprecated flag:
Screen Shot 2021-03-17 at 9 54 54 AM

New flag:
Screen Shot 2021-03-17 at 9 52 56 AM

No flag:
Screen Shot 2021-03-17 at 9 54 12 AM

The format flag is a string that currently only takes in JSON as a valid input option.
It's inteded to replace the --print-json for the listen cmd. The difference in behavior between
--format JSON and --print-json is that the new flag will stripe out all formatting in the payload.
Copy link
Collaborator

@vcheung-stripe vcheung-stripe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some suggestions for the output strings 😄

pkg/cmd/listen.go Outdated Show resolved Hide resolved
pkg/cmd/listen.go Outdated Show resolved Hide resolved
@gracegoo-stripe
Copy link
Contributor Author

I have some suggestions for the output strings 😄

Oh my goodness, thank you for catching these!

Copy link
Collaborator

@vcheung-stripe vcheung-stripe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The messages look good to me. I can't speak for the rest of it so passing back r? @tomer-stripe

Copy link
Collaborator

@tomer-stripe tomer-stripe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good! Two quick questions:

  1. is JSON case sensitive? It probably shouldn't be but I can't remember what we do in request logs
  2. did you test this with piping into jq? We should make sure that still works (and potentially mention that in the deprecation message too)

@pepin-stripe
Copy link
Contributor

LGTM but same questions as @tomer-stripe for case sensitivity. As a user I think I'd default to --format json since I'm usually used to pass in arguments in lower case

@gracegoo-stripe
Copy link
Contributor Author

This looks good! Two quick questions:

  1. is JSON case sensitive? It probably shouldn't be but I can't remember what we do in request logs
  2. did you test this with piping into jq? We should make sure that still works (and potentially mention that in the deprecation message too)
  1. They are not but I can update them!
  2. I did just now and that does work. I can suggest this in the deprecation message as well, that's a good idea.

Screen Shot 2021-03-18 at 9 39 05 AM

@gracegoo-stripe
Copy link
Contributor Author

gracegoo-stripe commented Mar 18, 2021

Okay, I updated the flag check to be case insensitive for both listen and logs tail. Also added colorized output to the listen output because that looked fun:

Screen Shot 2021-03-18 at 11 55 58 AM

Screen Shot 2021-03-18 at 11 53 28 AM

And the updated deprecation message:

Screen Shot 2021-03-18 at 11 59 00 AM

ptal @tomer-stripe

Copy link
Contributor

@pepin-stripe pepin-stripe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOVE IT!

@gracegoo-stripe gracegoo-stripe merged commit fb121ab into master Mar 18, 2021
@gracegoo-stripe gracegoo-stripe deleted the gracegoo-listen-cmd-format-flag branch March 18, 2021 22:34
az777777 pushed a commit to az777777/stripe-cli that referenced this pull request Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants