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

feat(webhooks): add oclif version of webhooks plugin #1253

Merged
merged 42 commits into from
Aug 21, 2019

Conversation

chadian
Copy link
Contributor

@chadian chadian commented Jun 17, 2019

This pull request migrates the webhooks-v5 plugin to a version written in oclif.

@chadian chadian requested a review from a team June 17, 2019 20:20
@chadian chadian self-assigned this Jun 17, 2019
@chadian
Copy link
Contributor Author

chadian commented Jun 18, 2019

I think thrown errors show a stacktrace unless this.error is used? Calls to webhookType could be wrapped with a try/catch to catch the error thrown from webHookType when --app or --pipeline isn't specified.

    let path
    try {
      let type = webhookType(flags)
      path = type.path
    } catch (e) {
      this.error(e.message)
    }

Which would give us:

./bin/run webhooks:events:info
 ›   Error: No app specified

instead of the current behavior:

Error: No app specified
    at Object.default_1 [as default] (~/projects/heroku/cli/packages/webhooks/src/webhook-type.ts:19:9)
    at Info.run (~/projects/heroku/cli/packages/webhooks/src/commands/webhooks/events/info.ts:29:31)
    at Info._run (~/projects/heroku/cli/node_modules/@oclif/command/lib/command.js:34:31)

@chadian chadian force-pushed the cc/add-oclif-webhooks-plugin branch from 093aa50 to 386eed5 Compare July 18, 2019 20:50
chadian and others added 23 commits July 31, 2019 16:01
- Add cli-ux and @heroku-cli/color packages
- Migrate webhookType helper
Tests can be an issue when using cli-ux features that use oclif/screen.
Depending on the width of the terminal the output might wrap or truncate.
To get around this we set the global `columns` value, which is checked by
oclif/screen. This specifically impacted tables and error message
wrapping. 140 characters appears to be a width that works for the tests
currently.

See https://github.com/oclif/screen
@zwhitfield3 zwhitfield3 force-pushed the cc/add-oclif-webhooks-plugin branch 2 times, most recently from aa3d7f3 to 3818dd4 Compare August 7, 2019 22:10
@buildsize
Copy link

buildsize bot commented Aug 7, 2019

File name Previous Size New Size Change
bundlesize.tar 65.48 MB 70.02 MB 4.54 MB (7%)

@zwhitfield3 zwhitfield3 force-pushed the cc/add-oclif-webhooks-plugin branch from a1afd28 to b84bc6c Compare August 8, 2019 00:15
Copy link
Contributor

@RasPhilCo RasPhilCo left a comment

Choose a reason for hiding this comment

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

I left this comment on a two commands but it check all commands:

  1. use flags.pipeline from @heroku-cli/command
  2. make sure the arg ID is required

@zwhitfield3 zwhitfield3 requested a review from RasPhilCo August 12, 2019 20:21
Copy link
Contributor

@RasPhilCo RasPhilCo left a comment

Choose a reason for hiding this comment

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

🏄

@RasPhilCo
Copy link
Contributor

cc @ransombriggs just a heads-up on this port to v7

@RasPhilCo RasPhilCo merged commit 110c516 into master Aug 21, 2019
@RasPhilCo RasPhilCo deleted the cc/add-oclif-webhooks-plugin branch August 21, 2019 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants