-
Notifications
You must be signed in to change notification settings - Fork 56
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
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 . 😊
There was a problem hiding this comment.
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
chorescripts and thelerna-changelogpackage in this PR would be removed but thepostpublishscript would remain.This would:
@snewcomer I'd appreciate your thoughts. 😊
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.ciignorefile to not fail CI for like,.mdfiles without a semver version bump.Even after implementing Harmless Changes,
package.jsonupdates can be harmful—sopackage.jsonupdates would require a version bump...if this addon starts to leverage automation opportunities which I hope people aren't against.There was a problem hiding this comment.
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.