Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: Add missing date-time format to time fields #655

Merged
merged 1 commit into from Jul 1, 2022
Merged

feat: Add missing date-time format to time fields #655

merged 1 commit into from Jul 1, 2022

Conversation

Helcaraxan
Copy link
Contributor

While doing some parsing & codegen work based on the JSON specs I realised that some time-based fields were missing the date-time format validation which is a very nice-to-have when dealing with unmarshalling into typed languages.

This PR's changes solely focus on adding those missing format hints.

@ghost ghost added this to Inbox in JS Jun 30, 2022
@gr2m gr2m added Type: Bug Something isn't working as documented typescript labels Jun 30, 2022
@ghost ghost moved this from Inbox to Bugs in JS Jun 30, 2022
wolfy1339
wolfy1339 requested changes Jun 30, 2022
View reviewed changes
Copy link
Member

@wolfy1339 wolfy1339 left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a little nitpick, can you have the null as the last value in the oneOfs.

It would avoid having to rebuild the types

@Helcaraxan
Copy link
Contributor Author

Of course @wolfy1339. Sorry about that oversight. Should be fixed now.

@Helcaraxan
Copy link
Contributor Author

By the way should I rename the PR to get a bug: prefix for semver? I see that you applied the Bug label to this. I used the feature-level one as I was not sure how the project considers adding validation logic with respect to semver.

@gr2m
Copy link
Contributor

gr2m commented Jul 1, 2022

@wolfy1339 I don't think this changes anything for typescript though, other than https://github.com/octokit/webhooks/pull/655/files#diff-1680c176c64b33118d8088f64fbd82ff896d471c6d18e1d9911ef9aa695c7b0eL3429, right? I just ask because you added the typescript label? This pull request is just improving the JSON Schemas, which I agree is a feature, as it's an addition?

@wolfy1339
Copy link
Member

wolfy1339 commented Jul 1, 2022 •
edited

By the way should I rename the PR to get a bug: prefix for semver?

It's fine, we can change it if needed when we merge the PR

I don't think this changes anything for typescript though, other than https://github.com/octokit/webhooks/pull/655/files#diff-1680c176c64b33118d8088f64fbd82ff896d471c6d18e1d9911ef9aa695c7b0eL3429, right?

That would be correct.

I just ask because you added the typescript label?

I didn't label this PR, you did 😆

@gr2m gr2m added Type: Feature New feature or request and removed Type: Bug Something isn't working as documented typescript labels Jul 1, 2022
@ghost ghost moved this from Bugs to Features in JS Jul 1, 2022
@Helcaraxan
Copy link
Contributor Author

Ah no, not me either, it was @gr2m. 😆

I did just fix the linting issue. Apologies again, first time contributing to the project need to get the specifics right.

@wolfy1339
Copy link
Member

I did just fix the linting issue. Apologies again, first time contributing to the project need to get the specifics right.

No worries! You're doing just fine for a first time with this project

@wolfy1339 wolfy1339 merged commit b6bad6a into octokit:master Jul 1, 2022
JS automation moved this from Features to Done Jul 1, 2022
@Helcaraxan Helcaraxan deleted the dev/time-formats branch July 1, 2022 19:02
@Helcaraxan
Copy link
Contributor Author

Thank you both @wolfy1339 and @gr2m for getting this merged. 🙂

@octokitbot
Copy link
Collaborator

🎉 This PR is included in version 6.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wolfy1339 wolfy1339 wolfy1339 approved these changes

Assignees
No one assigned
Labels
Type: Feature New feature or request
Projects
No open projects
JS
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Helcaraxan @gr2m @wolfy1339 @octokitbot