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: add automatic changelog generation #225

Merged
merged 1 commit into from
Mar 5, 2019

Conversation

rjoaopereira
Copy link
Contributor

#224

Asking for comments on this.

A possible solution for the issue.

Added support for automatic changelog generation through conventional-commits and standard-version.

Also added linting for pre-commit and commit-msg.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@dbanksdesign
Copy link
Member

I like this a lot, I just need to kind of wrap my head around it a bit. Are there other alternatives, and why is this way the best? It would be nice to also update the contributing docs to talk about how to commit properly using this new way, both for future contributors and for my own edification as well 😄

@rjoaopereira
Copy link
Contributor Author

Other options are available such as semantic-release. Some info on the differences.

For linting there is also precise-commits

@rjoaopereira rjoaopereira force-pushed the feat/changelog branch 2 times, most recently from b51bd8d to 5a526d9 Compare January 15, 2019 10:29
@rjoaopereira
Copy link
Contributor Author

I added a small reference in CONTRIBUTING.md to commit rules.
These basically just point to the conventional commits specification.

@chazzmoney
Copy link
Collaborator

I think this could be good - and it seems like a two-way door. If it doesn't work for us, we could back this change out easily.

@didoo
Copy link
Contributor

didoo commented Jan 19, 2019

Two things I am not sure about:

Please follow the spec in order to have a successful commit.

I have just submitted a PR, and I had no idea of what tags to use (also, I have to remember to do it in every commit). I am not sure about putting this onus on the contributors, this could mean creating a remarkable barrier to entry.

squash commits when merging pull requests

Personally, I prefer to see the history of the commits, to see the evolution of a feature, see the changes and why they've been done, etc. - I can live with it, anyway :)

@rjoaopereira
Copy link
Contributor Author

I understand your concerns. Let me see if I can help my case.

I have just submitted a PR, and I had no idea of what tags to use

Generally, fixand feat are the common ones. BREAKING CHANGE is mandatory for a major.
The normal config follows angular's norm.

also, I have to remember to do it in every commit

husky will remind you in every commit if you don't follow the spec. It just won't let you do it.

Personally, I prefer to see the history of the commits, to see the evolution of a feature, see the changes and why they've been done, etc.

I have lived in both ends of the spectrum and I prefer to have a clean history. Project management becomes easier. However, there are drawbacks like you say.

this blogpost does a good job of explaining the pros and cons.

@rjoaopereira
Copy link
Contributor Author

Kent C. Dodds made a nice course on egghead regarding open source js libraries.

A nice watch with reference to changelogs, commits and releases

@dbanksdesign
Copy link
Member

Apologies this is taking a bit longer. I'm diving more deeply into this. One issue with this (not your fault or the fault of conventional commits or anything) is we have an issue with our documentation generation step. In formats we have less formats, and each format has an @example block in the JSDoc which shows an example of the output of the format. Less uses the '@' character for variables, which breaks the jsdoc parsing because it thinks it is a JSDoc tag. I have an open issue on the JSDoc library, which has not been responded to.. 😔

I'll try to do some more digging around..

Thank you for sending me that course, I watched it yesterday and it was very helpful.

@davixyz
Copy link
Contributor

davixyz commented Feb 6, 2019

I love this; Definitely automating the changelog through Conventional Commits is a big win.
Here's a nicer to the eyes spec of the Conventional Commits:
https://www.conventionalcommits.org/en/v1.0.0-beta.3/#specification

also, to defend @rjoaopereira. If it's absolutely necessary, people that contribute to this library can keep writing the commits how they want. It's the job of the maintainers to add the correct tag like fix, feat, etc when they merge squash the commits.

Copy link
Member

@dbanksdesign dbanksdesign left a comment

Choose a reason for hiding this comment

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

Apologies for taking so long on this, I think we are about ready for this. If you could rebase your branch from master to get #240 in there and take a look at the other comment, we are good to go! Thank you again for your time and patience on this.

"test": "npm run lint && jest --runInBand",
"test-watch": "npm run lint && jest --runInBand --watch",
"coverage": "istanbul cover ./node_modules/mocha/bin/_mocha -- test/*.js test/**/*.js",
"preversion": "npm test",
"version": "node ./scripts/version.js && npm run generate-docs",
Copy link
Member

Choose a reason for hiding this comment

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

Could we still call the ./scripts/version.js file somehow in the release? All that file is doing is updating the version number of style dictionary in the examples to be the new version number. Or if there is a better way to do this lets do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Calling it and doc generation after version bump.
A suggestion would be to create a monorepo with lernajs and make examples a package that depends on style-dictionary. When bumping SD, all others would be updated. But this is another task

Copy link
Contributor Author

@rjoaopereira rjoaopereira Feb 21, 2019

Choose a reason for hiding this comment

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

you can call npm run release -- --dry-run to test things out.

Copy link
Member

Choose a reason for hiding this comment

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

Ohhh using lerna is a good idea!

Copy link
Member

@dbanksdesign dbanksdesign left a comment

Choose a reason for hiding this comment

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

LGTM! :shipit:

@chazzmoney chazzmoney merged commit b062008 into amzn:master Mar 5, 2019
@chazzmoney chazzmoney mentioned this pull request Mar 5, 2019
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

5 participants