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.

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

Fixing issue 105 date-time format validation problem #106

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dezfowler
Copy link

@dezfowler dezfowler commented Sep 23, 2017

Fixing issue #105 which allowed timezone offset to be omitted which is not valid under RFC 3339.

'K' in the format string used will match no timezone.

@JamesNK
Copy link
Owner

JamesNK commented Sep 23, 2017

Hi

Thanks for looking at this. I'm going to need to think carefully about this change because it could introduce validation errors to current users when they upgrade. One option could be to include a JsonValidator that validates date-time strictly. Example here - https://www.newtonsoft.com/jsonschema/help/html/CustomJsonValidator.htm

What do you think?

@dezfowler
Copy link
Author

Yeah, as highlighted by those broken tests elsewhere in the solution. I kinda left those broken intentionally to highlight this very issue. I was thinking some kind of strict mode would probably be the way forward here.

Given the various drafts of the spec it probably makes sense to have some profiles which just add a particular set of validators.

I've actually identified an additional issue with the date format not covered by my fix and, for my use case, I ideally want to disable the string formats which aren't part of the spec.

@CLAassistant
Copy link

CLAassistant commented Oct 3, 2019

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


DerekF seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants