-
Notifications
You must be signed in to change notification settings - Fork 127
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(package): Add support for missing version #25
Conversation
Hey @ffflorian, thanks for the contribution! I'd like to support this use-case, but could you help me understand why a
I'm not sure saying |
Hi @robinjoseph08!
At Wire we are looking for a Node.js tool which can generate changelogs - your tool looks like the perfect solution for our needs, but as mentioned before, it breaks in some of our projects. Furthermore IMHO it should at least display an error message if this breaks. 🙂
That sounds like a good idea, I will update my pull request accordingly. |
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.
The code looks good, but could you add some tests to confirm this logic? I want to make sure no regressions happen in the future. Thanks!
Sure, I'll update my PR soon. 🙂 |
@robinjoseph08 |
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.
Some last few nit picks, but then it'll be ready to merge!
test/package.test.js
Outdated
|
||
return Package.calculateNewVersion() | ||
.then(function (version) { | ||
Expect(version).to.eql(null); |
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.
Nit pick: can you change this to .to.be.null;
. We prefer to use the chai shorthands whenever possible.
test/package.test.js
Outdated
.then(function (version) { | ||
Expect(version).to.eql(null); | ||
}); | ||
}); |
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.
Nit pick: can you add a new line after this it
block? We usually pad our it
blocks with new lines on the top and bottom.
test/writer.test.js
Outdated
.then(function (changelog) { | ||
var heading = changelog.split('\n')[0]; | ||
|
||
Expect(heading).to.equal('## ' + new Date().toJSON().slice(0,10)); |
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.
Nit pick: can you add a space after the comma in the .slice
? There should've been a lint rule for that. I'll look into why that didn't err out.
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.
Actually I copied this from writer.js but I see that you updated it with the new lint version :)
@ffflorian I found out that the version of our linter that this repo was using was a bit outdated, so I updated it in #26. You should rebase off of master and you'll get the changes! |
Everything done 🙂. Should I rebase my PR into one commit? |
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.
LGTM! The rebase isn't necessary because I'll use the "Squash and Merge" feature that GitHub has. Thanks for asking though!
Thanks again for this contribution! I'll let you know when a new version of the module has been released.
v1.4.0 has been published! Thanks again! |
Thanks @robinjoseph08! 🙂 |
This commit adds support for projects without a
version
field in thepackage.json
file.Fixes #24.