jsonresume / resume-schema

JSON-Schema is used here to define and validate our proposed resume json
http://jsonresume.org
MIT License
2.15k stars 278 forks source link

fix: change certificates date format to ISO8601 #456

Closed hugginsio closed 1 year ago

hugginsio commented 1 year ago

This PR modifies the date property of the certificates object to use the ISO8601 reference defined in the schema.

These changes resolve #412 and #448.

hugginsio commented 1 year ago

@SethFalco you are the most recent maintainer to merge a PR to this repository - are you the correct person to reach out to for a review?

SethFalco commented 1 year ago

Hey hey!

Yes and no! I contribute/maintain the repos occasionally, but I do everything except merge changes to the schema itself!

Unfortunately, for that one we'll need to wait for one of the OG maintainers to have time to check this out. Though this does look like a fairly non-controversial change.

Looking at the git history, it does look to me like the ISO8601 definition was just introduced later, and that instance was missed when migrating. :thinking:

@thomasdavis Would you care to give this a peek? In practice, it shouldn't change anything, it just makes the schema enforce the documented behavior.

SethFalco commented 1 year ago

I was wondering what's the point in the iso8601 definition, but I see now it's because the built-in date mandates year, month, and day fields, while we wanted month and day to be optional. :thinking:

I also see an open issue with your suggested changes which were already endorsed. (Which you also linked already. :facepalm: Should've checked that.)

This is a great find, it should definitely be consistent, thanks!

https://github.com/jsonresume/resume-schema/issues/448#issuecomment-1274575093

Just to play it safe, I'm going to verify the tests and make sure nothing explodes, and may check if it's possible to leave a comment or note that explains why we define our own iso8601, when date itself is documented to handle ISO8601 already.

Thanks for PR meanwhile, hopefully Thomas has a chance to check this out, but if not I'll opt to act on my own next week since I'm pretty confident this is fine.

hugginsio commented 1 year ago

Seth, thank you for your quick response (and transparency!), as well as pulling in the right maintainer for review. I will likely keep any future pull requests limited to endorsed issues, if there are any open to address. Thank you again for taking a look at this particular PR.

I appreciate what you and the other maintainers have built with this project and I hope to be able to consolidate my resumes into this schema.

SethFalco commented 1 year ago

Turns out we had no tests for the certificates portion of the schema, so just updated the PR to include the tests. I also added a comment to explain why iso8601 is defined.

Thanks for pointing this out!

github-actions[bot] commented 4 months ago

:tada: This PR is included in version 1.1.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: