-
-
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
[CTAN] fallback to date if version is empty #9036
Conversation
|
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 for picking this up :) Couple of minor comments
services/ctan/ctan.service.js
Outdated
|
||
const schema = Joi.object({ | ||
license: Joi.array().items(Joi.string()).single(), | ||
version: Joi.object({ | ||
number: Joi.string().required(), | ||
number: Joi.string().allow('').required(), | ||
date: Joi.string().allow(''), |
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.
if we expect this to be present in the response but set to empty string to indicate "not set" then yes it should be .required()
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.
In https://github.com/muzimuzhi/shields/commit/80c1fe94c9106659ec010d6b5247edb103f8a377 I added .required()
to date
without adding date
entry to tests. Surprisingly, all checks passed. Is this the expected result?
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.
Seems it's because not all workflows are triggered by pushing to my fork. date
entries are required.
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.
nice LGTM 👍
I'm new to node.js, hence this PR is more or less an imitation of existing code in the same repo.
Do you suggest to make
version.date
inschema
also required?Update: I did a test in https://github.com/muzimuzhi/shields/commit/80c1fe94c9106659ec010d6b5247edb103f8a377. Surprisingly, without adding
date: "..."
to examples and tests, all checks passed.Fixes #9017