-
Notifications
You must be signed in to change notification settings - Fork 713
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
[WIP] feat(link-compare): generate compare url and simplify templates #187
[WIP] feat(link-compare): generate compare url and simplify templates #187
Conversation
- generate compare url when there are tags - added test for bitbucket host - updated templates to use `linkCompare` compare directly
*/ | ||
function generateCompareUrl(context) { | ||
var generatedUrl = ''; | ||
if (context.repository) { |
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.
Logic extracted from hbs templates
pkg: { | ||
path: __dirname + '/fixtures/_bitbucket-host.json' | ||
}, | ||
config: require('../../conventional-changelog-angular') |
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.
Referenced the local changelog-angular
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.
Is this the only location in conventional-changelog-core
that we test for a valid compare
link?
Should this be extended to also test for GItLab, GitHub, and a unknown SCM platform?
Thanks for the PR. Could you please have a look at conventional-changelog-archived-repos/conventional-changelog-core#21 (comment)? |
I ran through that thread as well. I'm in the opinion of not adding helpers since most of the logic for generating the I'm also sort of new to the library, so I'm not too sure of all the capabilities that conventional changelog provides. It looks like you can feed in your own handlebars template and in that case it might be reasonable to provide helpers for the end user to use. |
@stevemao ? Anything I should adjust here or think about here? I tried posting this in the other thread as well. |
Sorry @admosity I haven't got time for this. I feel like the current API of writer is not good. Adding feature like this (with either approach I mentioned before) is supposed to be easy, but it's not 😢 . See conventional-changelog-archived-repos/conventional-changelog-writer#20 |
* @returns {string} compare url for gitlab | github | bitbucket | ||
*/ | ||
function generateCompareUrl(context) { | ||
var generatedUrl = ''; |
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.
Could you switch to let
over var
, and switch to template literals over single quotes?
@@ -212,7 +245,8 @@ function mergeConfig(options, context, gitRawCommitsOpts, parserOpts, writerOpts | |||
|
|||
context = _.assign({ | |||
issue: hostOpts.issue, | |||
commit: hostOpts.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.
I wish the style rules for this repository allowed for trailing commas so that these lines wouldn't need to be touched to add that comma for a new line underneath. (Just my thoughts on the matter. Nothing actually needs to change here)
pkg: { | ||
path: __dirname + '/fixtures/_bitbucket-host.json' | ||
}, | ||
config: require('../../conventional-changelog-angular') |
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.
Is this the only location in conventional-changelog-core
that we test for a valid compare
link?
Should this be extended to also test for GItLab, GitHub, and a unknown SCM platform?
Didn't know there was movement here. @hbetts I'll get around to reviewing what you've brought up soon. |
Cross referencing #185 |
I am closing this pull request for now. Please feel free to re-submit the pull request if you have time to make progress on this work. |
@admosity did you get a chance to look into this? |
linkCompare
compare directlyImproves #80
Fixes #185, conventional-changelog/standard-version#142
Didn't go the template editing route because of:
handlebars-lang/handlebars.js#927
and
http://chrismontrois.net/2016/01/30/handlebars-switch/
Prior work:
conventional-changelog-archived-repos/conventional-changelog-core#21