Skip to content
This repository has been archived by the owner on Jun 27, 2019. It is now read-only.

[MAJOR] json format only outputs valid json #260

Merged
merged 7 commits into from
May 4, 2018
Merged

[MAJOR] json format only outputs valid json #260

merged 7 commits into from
May 4, 2018

Conversation

xavdid
Copy link
Contributor

@xavdid xavdid commented Mar 2, 2018

In order for command output to be piped into other commands (such as jq, a json formatter), it can't print extra helpful lines or other, non-json text.

The fix is twofold. The easier side is not printing "these are your apps:" and similar. that's a single check in the context.line function.

The more complex half is our custom spinner implementation, which prints out hidden characters when it starts and stops (not to mention, it's very complex), invalidating the output. Moved the spinner to an external lib that's well maintained (ora). initial results seem awesome but there's still a lot of tire-kicking to do there.

fixes #247 and PDE-89

@xavdid xavdid requested a review from eliangcs March 6, 2018 09:19
@xavdid xavdid changed the title [WIP] json format only outputs valid json json format only outputs valid json Mar 6, 2018
@xavdid
Copy link
Contributor Author

xavdid commented Mar 6, 2018

We now have a new spinner and most of the commands that have a table can output data as a pure json structure. To test, pipe the output of a command into jq: zapier versions --format=raw | jq, which should not error.

Internally, the spinner API is much simpler. All we use now is startSpinner and endSpinner, which takes a message and a success boolean, respectively. The latter also takes a message, but that replaces the started message, which is pretty confusing; i'm thinking of removing it as we don't use it anywhere. We removed a bunch of calls to clearSpinner(), since we're no longer worried about things left behind inappropriately, and we don't have to worry about the spinner being there after the cli is exited.

I also removed a param in callAPI which we no longer need (because individual commands can choose how to handle errors before bubbling them up to entry).

Notes:

  • all of the commands in this comment besides describe and validate. They both output multiple tables, so if we want those to be real json, we'll need to change the actual structure of the method. We can do that, but it seems out of scope for what people will probably need this functionality for.
  • through some black magic, the spinner running and showing messages doesn't affect the validity of the json output. to see this in action, run zapier logs --format=raw | jq, which has a spinner on the screen that succeeds, then outputs valid json anyway. neat!

Copy link
Member

@eliangcs eliangcs left a comment

Choose a reason for hiding this comment

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

Awesome change! I did a quick test and didn't find any issues. The code is much cleaner now, too!

@xavdid
Copy link
Contributor Author

xavdid commented Mar 6, 2018

Sweet! So the downside here is that this is a breaking change, which would require a 6.0 release. I'd love to do that only for CLI and not core/schema, per our discussion.

@eliangcs
Copy link
Member

eliangcs commented Mar 7, 2018

Why a breaking change? Is it because we're not printing a context line like "these are your apps:" anymore?

@xavdid
Copy link
Contributor Author

xavdid commented Mar 7, 2018

That was my thought. If they had been parsing json out of the output before, we might break stuff. I like to be defensive about that, but we could also just ping the channel and go for it

@ibolmo
Copy link
Contributor

ibolmo commented Mar 7, 2018

I'm all in favor of just incrementing major. Our public contract is SEMVER, and it's just easier to adhere to it, then to break it.

@xavdid xavdid changed the title json format only outputs valid json [MAJOR] json format only outputs valid json Mar 28, 2018
@xavdid xavdid merged commit e42fd2c into master May 4, 2018
@xavdid xavdid deleted the valid-json branch May 4, 2018 05:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Commands that allow a "--format" argument return invalid JSON
3 participants