-
-
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
[GithubRelease],[GithubTag] filter by prefix #8205
Conversation
Thanks for the PR @mvndaai!
This is true but unrelated to the addition of this PR, and more just another impact of the fact that we don't support walking the pages for releases/tags yet (refs #6766); I wouldn't worry about this |
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.
Thanks again, few items left inline for your review
services/github/github-tag.tester.js
Outdated
@@ -28,6 +28,10 @@ t.create('Tag (repo not found)') | |||
.get('/v/tag/badges/helmets.json') | |||
.expectBadge({ label: 'tag', message: 'repo not found' }) | |||
|
|||
t.create('Tag (filter by prefix + trim prefix at slash)') | |||
.get('/v/tag/ros/rosdistro.json?subpackage=galactic/&trimPrefixAtSlash') |
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.
As I was reading through the diff again I was starting to feel like I'd be more comfortable with a greater number of tests, and I definitely feel that way after getting to this test, which is using the no-longer valid/existent query parameter name.
I think it would be helpful to try to move more of the logic out of the handle
function(s) (which are more difficult to test) into standalone functions (either class members or free functions) that can be directly unit tested and consumed by handle
and/or fetch
functions as needed
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 fixed that test. I had assumed that Joi.date().required()
meant that the string had to be only a date, not just include one. Since all that repo's tags are something like galactic/2022-10-04
it matched on all tags, which is disappointing because originally I had made a regex to just match the date but switched to Joi.date
instead on feedback. Now It checks for a date and a prefix of 20
.
I just reviewed the code and I don't know how much more I can break up. I could maybe make a function of splitting the prefix when you want to trim but that doesn't feel complex enough to need a function. If you have some pointers I would gladly take them.
Thanks for all the time you have spent reviewing this!
Hello. We have just shipped a new feature which allows you to pass an arbitrary filter to the github tag and release badges. For example The e.g: This feature should be powerful and flexible enough to enable this use-case (and a bunch of others). |
This change allows:
refPrefix
query