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

[init] adds initial automation of changelog.md, fixes changelog script #109

Merged
merged 1 commit into from
Apr 5, 2018

Conversation

yowainwright
Copy link
Contributor

@yowainwright yowainwright commented Apr 3, 2018

Automates CHANGELOG.md, git tags

@@ -1,6 +1,6 @@
{
"name": "ember-router-scroll",
"version": "0.6.0",
"version": "0.7.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we releasing a new version just to add changelogs?

Copy link
Contributor Author

@yowainwright yowainwright Apr 3, 2018

Choose a reason for hiding this comment

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

Are changelogs/tags and automating them not a new feature?

Copy link
Contributor

Choose a reason for hiding this comment

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

No. It's not a feature that is shipped to users, so users will think 0.7.0 offers them something, and it does not.

Copy link

Choose a reason for hiding this comment

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

Not only is this not a new feature from the perspective of the published package, but this version bump also marks it as a breaking change. You're signaling to anyone using this addon that they need to come do research to understand what's different before they can safely upgrade.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will get feedback from the people I requested code review from. They may agree with you @rwwagner90 . 😊

Copy link
Contributor Author

@yowainwright yowainwright Apr 3, 2018

Choose a reason for hiding this comment

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

I consulted with some teammates. They suggested removing the CHANGELOG.md all together. That would mean the chore scripts and the lerna-changelog package in this PR would be removed but the postpublish script would remain.

This would:

@snewcomer I'd appreciate your thoughts. 😊

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless I am misinformed about semver, we don't need to change the version at all here or release anything. It's just manipulating scripts and changelogs for people developing the addon, not for consumers of the addon. Changing the version of a package signals to the consumers that something changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

That being said, I do like changelogs, and I am fine with scripts to update them, I just don't think we want to change the version of this package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, so the people communicating in this PR want this PR but with no npm version bump?


Future note

If this addon uses CircleCi (which will allow for more community support), npm publishing can be automated...but will need semver version bumps to publish w/o failure.

That said, my team has built a script to ignore files that are deemed "harmless" within a repo—harmelss changes. This package could be implemented with a .ciignore file to not fail CI for like, .md files without a semver version bump.

Even after implementing Harmless Changes, package.json updates can be harmful—so package.json updates would require a version bump...if this addon starts to leverage automation opportunities which I hope people aren't against.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just don't think we should version things that are not versions. Once these processes are in place, all the changes should be actual functional ones, but might not always be. If changes are made that do not effect the consumer and do not change functionality, we should allow for a way to not version.

@yowainwright yowainwright requested a review from jakiestfu April 3, 2018 17:10
Copy link

@arjansingh arjansingh left a comment

Choose a reason for hiding this comment

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

🚢

@yowainwright yowainwright merged commit 4150589 into master Apr 5, 2018
@yowainwright yowainwright deleted the automate-tags branch April 5, 2018 18:38
@yowainwright yowainwright mentioned this pull request Apr 9, 2018
5 tasks
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.

4 participants