-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Allow [GitHub] tag badge to sort by date #2157
Conversation
Generated by 🚫 dangerJS |
side issue: one of the tests was failing because the GH API now returns |
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.
👍 Changes look great to me.
Just left a minor note in regards to the regex, although this is fine to merge with whichever option you decide. 😄
@@ -15,9 +15,10 @@ const { | |||
module.exports = class GithubTag extends LegacyService { | |||
static registerLegacyRouteHandler({ camp, cache, githubApiProvider }) { | |||
camp.route( | |||
/^\/github\/tag(-?pre)?\/([^/]+)\/([^/]+)\.(svg|png|gif|jpg|json)$/, | |||
/^\/github\/(tag-?pre|tag-date|tag)\/([^/]+)\/([^/]+)\.(svg|png|gif|jpg|json)$/, |
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.
Do you think it would be better to make the -
required for tag-pre
?
-/^\/github\/(tag-?pre|tag-date|tag)\/([^/]+)\/([^/]+)\.(svg|png|gif|jpg|json)$/
+/^\/github\/(tag-pre|tag-date|tag)\/([^/]+)\/([^/]+)\.(svg|png|gif|jpg|json)$/
or an alternative:
/^\/github\/tag(?:-(pre|date))?\/([^/]+)\/([^/]+)\.(svg|png|gif|jpg|json)$/ (- required)
/^\/github\/tag(?:-?(pre|date))?\/([^/]+)\/([^/]+)\.(svg|png|gif|jpg|json)$/ (- optional)
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 was keeping this to avoid making a breaking change:
/^\/github\/tag(-?pre)?\/([^/]+)\/([^/]+)\.(svg|png|gif|jpg|json)$/, |
..but now you mention it, one of the issues here was we've never put an example of this on the home page so I doubt there is much usage in the wild. It was only added quite recently. Lets change it now 👍
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.
Ah yes, Good point!
@@ -13,13 +13,13 @@ const { | |||
module.exports = class GithubRelease extends LegacyService { | |||
static registerLegacyRouteHandler({ camp, cache, githubApiProvider }) { | |||
camp.route( | |||
/^\/github\/release\/([^/]+\/[^/]+)(?:\/(all))?\.(svg|png|gif|jpg|json)$/, | |||
/^\/github\/release(-?pre)?\/([^/]+\/[^/]+)(?:\/(all))?\.(svg|png|gif|jpg|json)$/, |
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.
Do you think it would be better to make the -
required for tag-pre
?
-/^\/github\/release(-?pre)?\/([^/]+\/[^/]+)(?:\/(all))?\.(svg|png|gif|jpg|json)$/,
+/^\/github\/release(-pre)?\/([^/]+\/[^/]+)(?:\/(all))?\.(svg|png|gif|jpg|json)$/,
Catching up on some reading!
Could you clarify what you mean? I added some query param support in #1589. The handling of examples isn't completely correct as we've seen, and it's not stripping the default parameters, though the core of the query params are working in the the (new-style) NPM badges. |
Sorry - turns out I'm mistaken on this. I hadn't seen an example where we've used the |
Following on from #2117 (comment)
/release
badge so we can use/release-pre
and changed the home pages example. The legacy/all
suffix will still work (for backwards-compatibility) but its not documented./release
. Looking into it, we're using theprerelease
key there rather than assuming semver so we don't have the same issue there that we do with tags.tag-date
endpoint. I changed my mind and decided not to go withsort=semver/sort=date
for 2 reasons:/tag-pre/username/repo.svg?sort=date
which is a bit odd/unclearBaseJsonService
. This is going to be a problem with a few other services which we need to deal with when we port them, but on reflection I'd prefer not make this problem bigger.This PR won't change the behaviour for anyone already using these badges but improves consistency and provides a way for projects that don't use SemVer (for whatever reason) to use the
/tag
badge with sensible expected behaviour.Does this seem reasonable?