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(lifecycle): add support for lifecycle hooks stor… #167

Closed

Conversation

lancecaraccioli
Copy link

@lancecaraccioli lancecaraccioli commented Apr 12, 2017

Add hook support via "standard-version" package.json stanza

Closes #157

@coveralls
Copy link

coveralls commented Apr 13, 2017

Coverage Status

Coverage decreased (-2.9%) to 97.143% when pulling b168e9f on lancecaraccioli:issue-157 into 2429a68 on conventional-changelog:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-2.9%) to 97.143% when pulling b168e9f on lancecaraccioli:issue-157 into 2429a68 on conventional-changelog:master.

@coveralls
Copy link

coveralls commented Apr 13, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling e8a899b on lancecaraccioli:issue-157 into 2429a68 on conventional-changelog:master.

@Tapppi
Copy link
Member

Tapppi commented May 3, 2017

Hey, this is a great start! Sorry for the despicable turnaround time around standard-version lately, I think all the maintainers are really busy with projects and work right now.

I think the hooks should live in a package.json config stanza, as discussed previously in #157 (comment) . So the config would look like:

{
  "standard-version": {
    "hooks": {
      "pre-commit": "node hook.js",
      "post-bump": "validate-artifacts"
    }
  }
}

@stevemao @bcoe What do you think about hook config, and what should the names for the hooks be?

@lancecaraccioli
Copy link
Author

Yeah I like that approach. I think it feels comfortable to follow git hooks lead on naming as most dev's would probably feel comfortable with that (keeping the learning curve really low)

@bcoe
Copy link
Member

bcoe commented May 5, 2017

@lancecaraccioli @Tapppi I like the idea of lifecycle scripts 👍 but I agree with @Tapppi that I think they'd make sense in a hooks stanza of package.json.

We use the yargs command line parser which supports most of this functionality already:

https://github.com/yargs/yargs#pkgconfkey-cwd

@lancecaraccioli
Copy link
Author

lancecaraccioli commented May 5, 2017

I'll be working on updating the PR today.

Not sure about the yargs approach though. Seems like that would take some refactoring as cli.js imports command.js and then passes it to standardVersion as an argument, but there is no guarantee from the perspective of index.js that the argument will be a yargs object. Also it's current usage is as a plain object.

I'll think about it more. Thoughts?

@coveralls
Copy link

coveralls commented May 5, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 548d413 on lancecaraccioli:issue-157 into 2429a68 on conventional-changelog:master.

@lancecaraccioli
Copy link
Author

@Tapppi give this one another look please. Thanks

@lancecaraccioli lancecaraccioli changed the title feat(lifecycle): add support for git-hooks style lifecycle hooks stor… feat(lifecycle): add support for lifecycle hooks stor… May 5, 2017
@coveralls
Copy link

coveralls commented May 5, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 2d674d8 on lancecaraccioli:issue-157 into 2429a68 on conventional-changelog:master.

@bcoe
Copy link
Member

bcoe commented May 14, 2017

@lancecaraccioli this is looking great! sorry for the slow turnaround these days -- as @Tapppi has said himself, I think we're all buried under a few too many issues these days!

rather than the logic:

var hooks = pkg['standard-version'] && pkg['standard-version']['hooks'] ? pkg['standard-version']['hooks'] : {}

I'd love to see us use the pkgConf feature of yargs -- this is what I use for nyc, and yargs itself. It provides some extra logic around finding the appropriate package.json, regardless of how the consuming library is installed -- this should pretty much be a drop in replacement for you 😄

Copy link
Member

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

I'm really digging this work; really just one nit about using yargs' pkgConf feature.

I'm thinking that pulling configuration into the package.json could be the way we solve your other open issue surrounding configuring presets ... I just want to be really mindful about minimizing the amount of flexibility we add to standard-version within reason I want it to feel like magic, and push people towards a commit format that we advocate.

@lancecaraccioli
Copy link
Author

Hey @bcoe I'll try to give this a look soon, hopefully this evening. Thanks

@bcoe
Copy link
Member

bcoe commented May 27, 2017

@lancecaraccioli if you have any cycles soon; I'd love to get some of these issues closed out.

if (err) {
return done(err)
}
return tag(newVersion, pkg.private, args, done)
runLifecycleHook(args, 'pre-commit', newVersion, hooks, function (err) {
if (err) {
Copy link

Choose a reason for hiding this comment

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

Just a suggestion here, as this would help my own use case which I will outline below. If you allow the lifecycle hook to mutate/return a modified args struct that could allow more flexibility. (I'm currently considering using a prepare-commit-message hook to achieve this).

My company uses PivotalTracker which uses [(Fixes|Finishes) #12312] to indicate a story is fixed in a commit and then '[Delivers #12312]to mark it as delivered and a github integration to detect the commit message and update the stories. What I'd want to do in this feature is change themessage` argument depending on all "fixed" commits since last version. (It'd be awesome if standard-version/conventional-changelog could pass all the commits it builds the changelog off to the hook as well but that might be asking for too much).

Example

Git History:

fix: Some Issue [Fixes #123]
feat: Shiny Feature [Finishes #333]
chore(release): v1.1.2

On running standard-version I ideally want the release commit to be as follows so my stories automatically get updated to the delivered (for QA) state.

chore(release): v1.2.0

[Delivers #123]
[Delivers #333]

Assuming the hook allowed us to return a modified args I could append the new messages onto the message argument.

@revin
Copy link

revin commented Jun 2, 2017

Hi all, I work on marky-markdown; @bcoe recently helped get the project onto standard-version and suggested I throw my two cents in here.

This looks pretty great to me; in our case, for each release we build a small JSON file that includes the new package version number. We'd been doing that in the version npm run-script; I don't see a way to get standard-version to do that as is, but this PR looks like it would make that possible, so I'm into it 💯

In particular our use case is to have something run after the new version number is known, and to be able to programmatically generate changes to add to the commit that performs the version bump. So I'm guessing either of the post-bump or pre-commit hooks would work, yeah?

I don't have a strong or worthwhile opinion on decisions like how and where to specify the contents of the hooks themselves or anything; I'm just very interested in this feature overall 👍

@bcoe
Copy link
Member

bcoe commented Jun 5, 2017

@revin @SimeonC @lancecaraccioli please give this a shot:

npm i standard-version@next and see these docs:

https://github.com/conventional-changelog/standard-version#lifecycle-scripts

@SimeonC I tried to at least partially solve your use-case, you can provide a git message in the precommit hook -- perhaps you could look at the contents of CHANGELOG.md (which will have already been generated) to figure out the changes that have taken place?

@bcoe
Copy link
Member

bcoe commented Jun 6, 2017

@lancecaraccioli I've continued polishing the lifecycle script, documented here:

https://github.com/conventional-changelog/standard-version#lifecycle-scripts

Would love your help QAing, since the next release is fairly large, simply:

npm i standard-version@next

feryardiant added a commit to feryardiant/wpdev that referenced this pull request Oct 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

8 participants