Skip to content
This repository has been archived by the owner on Aug 22, 2023. It is now read-only.

feat: add postrun hook #111

Merged
merged 11 commits into from
Jun 9, 2020
Merged

feat: add postrun hook #111

merged 11 commits into from
Jun 9, 2020

Conversation

colbymatthiassonpublic
Copy link
Contributor

added post run hook capability

@salesforce-cla
Copy link

Thanks for the contribution! It looks like @colbymatthiassonpublic is an internal user so signing the CLA is not required. However, we need to confirm this.

@codecov
Copy link

codecov bot commented May 21, 2020

Codecov Report

Merging #111 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #111   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files           2       2           
  Lines          12      12           
  Branches        3       3           
======================================
  Misses         12      12           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 954ed2c...9e87536. Read the comment docs.

src/config.ts Outdated
@@ -355,6 +355,7 @@ export class Config implements IConfig {
const command = c.load()
await this.runHook('prerun', {Command: command, argv})
await command.run(argv, this)
await this.runHook('postrun', {Command: command, argv})
Copy link
Contributor

@amphro amphro May 21, 2020

Choose a reason for hiding this comment

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

We should capture the results of the command and pass that into the post run hook. I would imagine a lot of post hooks would want to try and do something with the command results.

withConfig
.stdout()
.it('runs ts command and prerun hooks', async ctx => {
.it('runs ts command and prerun and postrun hooks', async ctx => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Might as well have this as a separate test. Also, what about if the command errors? Add a test for that. I'm assuming the hook won't fire. Is that the right behavior?

@salesforce-cla
Copy link

Thanks for the contribution! It looks like @colbymatthiassonpublic @moberhauer is an internal user so signing the CLA is not required. However, we need to confirm this.

Copy link
Contributor

@amphro amphro left a comment

Choose a reason for hiding this comment

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

Just one comment otherwise looks great.

.stdout()
.it('runs ts command, postrun hook captures command result', async ctx => {
await ctx.config.runCommand('foo:bar:test-result')
expect(ctx.stdout).to.equal('running ts prerun hook\nit works!\nrunning ts postrun hook\nreturned success!\n')
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

package.json Outdated
@@ -50,6 +50,7 @@
"posttest": "yarn lint",
"prepack": "yarn run build",
"test": "mocha --forbid-only \"test/**/*.test.ts\"",
"hooktest": "mocha --forbid-only \"test/typescript.test.ts\"",
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't love this. The typescript.test.ts has a test other than hooks too. And if you need to run one file, just run mocha directly. Let's remove this.

Copy link
Contributor

@amphro amphro left a comment

Choose a reason for hiding this comment

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

Tested this out locally. 👍 🚢

src/hooks.ts Outdated Show resolved Hide resolved
Co-authored-by: RasPhilCo <[email protected]>
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.

Great PR! Few readability edits.

test/typescript.test.ts Show resolved Hide resolved
test/typescript.test.ts Show resolved Hide resolved
test/typescript.test.ts Show resolved Hide resolved
withConfig
.stdout()
.it('runs ts command and prerun hooks', async ctx => {
.it('runs ts command, prerun and postrun hooks both trigger', async ctx => {
Copy link
Contributor

Choose a reason for hiding this comment

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

how about? 'runs ts command and prerun & postrun hooks'

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

@colbymatthiassonpublic @moberhauer click the CLA link to get this status check approved

Thanks for the contribution! It looks like @colbymatthiassonpublic @moberhauer is an internal user so signing the CLA is not required. However, we need to confirm this.

@RasPhilCo RasPhilCo changed the title Post Run Hook feat: add postrun hook Jun 2, 2020
@amphro amphro merged commit 149529d into oclif:master Jun 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants