Add optional query parameter (include_prereleases) to [GemVersion] #6451
Conversation
|
There was a problem hiding this comment.
Hello @profgrammer ! 👋🏻
Thanks for your contribution to Shields.io! In my opinion, having the new optional parameter as a query parameter rather than an element in the path would be a better fit.
Do we foresee any values other than stable or latest in the near future? If not, we could even make it a "valueless" query parameter, e.g. https://img.shields.io/gem/v/formatador?latest or something appropriately named like that.
|
Hi @PyvesB, thank you for your inputs. I don't think we expect values other than |
|
In that case, I can point you to this implementation here: https://github.com/badges/shields/blob/master/services/youtube/youtube-likes.service.js Note the |
profgrammer
changed the title
|
I'm not terribly familiar with the Ruby ecosystem, but would it make sense to use the prerelease type naming/conventions we use in other version badges? |
|
@calebcartwright I see 2 options for this -
I think the latter would be a better option, will go ahead and rename the query parameter. Thanks! |
profgrammer
changed the title
There was a problem hiding this comment.
Thanks for polishing the implementation promptly @profgrammer, it mostly looks good to me at this point. 👍🏻
|
|
||
| // this is the same as isVPlusDottedVersionNClausesWithOptionalSuffix from test-validators.js | ||
| // except that it also accepts regexes like 5.0.0.rc5 - the . before the rc5 is not accepted in the original | ||
| const isVPlusDottedVersionNClausesWithOptionalSuffix = withRegex( |
There was a problem hiding this comment.
I'm wondering whether it would have been better to modify the original regex directly. Admittedly I've not seen many projects use a dot as the separator for the pre-release tag outside the Ruby ecosystem, so what you've done may make more sense. Let's leave it as is for now, but another maintainer will possibly have a stronger view on this one. :)
There was a problem hiding this comment.
generally agreed but also content with leaving this here for now. we probably do have some minor proliferation of duplicative regexes but I don't have any concerns with having this one here
There was a problem hiding this comment.
Looks good to me, thanks for the good work! Won't merge just yet in case @calebcartwright has some additional thoughts. :)
There was a problem hiding this comment.
LGTM as well. Thanks for this @profgrammer, nice first contribution to the Shields project!
Added an optional query parameter (include_prereleases) to the GemVersion service, with reference to #3222