Closed EvHaus closed 3 years ago
Merging #31 (1b46041) into main (dc91b18) will not change coverage. The diff coverage is
100.00%
.
@@ Coverage Diff @@
## main #31 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 1 1
Lines 425 521 +96
=========================================
+ Hits 425 521 +96
Impacted Files | Coverage Δ | |
---|---|---|
index.js | 100.00% <100.00%> (ø) |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update dc91b18...1b46041. Read the comment docs.
Great feedback @wooorm. I actually really wanted to provide all the additional variables but was worried it would make this PR too big. I'm happy that you're okay with that direction. I appreciate your support with this request.
I've pushed an update with your recommendations.
It did get me thinking though... since we're passing the full variables now, in theory we could allow users to replace the entire URL, not just the base URL. This would allow users to craft any URL they want when those links are clicked. This would actually be really useful in ZenHub's case (which I'm trying to support). The trade-off is that it would make the API more complex when the majority of the users of remark-github
probably don't care about this and just want basic http://github.com
URLs.
What do you think? Would you be okay with me changing the baseUrl
option to be a customUrl
option that allows users to replace the entire URL instead of only the base URL?
Yes, that's what I mean with my last example of 'buildUrl'! I think something along those lines is a good idea
@wooorm Alright. This is ready for a final review. Great new feature!
@wooorm Thanks for the feedback. Good points. I've addressed the feedback in a separate new commit so that it's easier to review.
@wooorm Fixed the 2 small nits. No other questions. This will work great for my needs.
Hi! This was closed. Team: If this was merged, please describe when this is likely to be released. Otherwise, please add one of the no/*
labels.
released!
Initial checklist
Description of changes
The PR adds support for a new
githubUrl
option. This is specifically to address #23 but it goes a little bit further than the original PR for it (#24) and allows the option to be a function. This is because, I work on the ZenHub team and we would love to use this package in our application but our application requires that the URL is sometimes going to github.com (such as for @ mentions) but other times it should go to zenhub.com (such as for issue references). By allowing the option to be a function, we can be add this customization while keeping theremark-github
package simple and easy to test.I've added documentation and tests for all the affected code paths. Coverage remains at 100%.