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

Fix logic error in format validation #80

Merged
merged 4 commits into from Jan 4, 2014
Merged

Fix logic error in format validation #80

merged 4 commits into from Jan 4, 2014

Conversation

jpmckinney
Copy link
Contributor

If you want to raise an error if !data.is_a?(String), you can't wrap that code in a if data.is_a?(String) block...

@jpmckinney
Copy link
Contributor Author

See the diff ignoring whitespace changes (since I had to unindent a lot) https://github.com/hoxworth/json-schema/pull/80/files?w=1

@hoxworth
Copy link
Contributor

Hi @jpmckinney - sorry it has taken me so long to get to this, but I'm trying to finally clean up the pull requests and issues on this project. This pull request is failing a few tests, and with the recent pull request merges there is now a merge conflict. Mind updating this PR?

@jpmckinney
Copy link
Contributor Author

Updated! Use this link to compare: https://github.com/hoxworth/json-schema/pull/80/files?w=1

@jpmckinney
Copy link
Contributor Author

The old version of the code had pseudocode like this:

if data is a String:
  raise an exception if data is not a String

Obviously, that exception will never be raised. I removed the if-statement, which caused that exception to start being raised, and which caused four tests to fail. I've restored the behavior expected by the tests by removing both the if-statement and the exception-raising.

@hoxworth
Copy link
Contributor

hoxworth commented Jan 4, 2014

Thanks for the cleanup - and yes, those validation errors were never going to be thrown, considering that code could never be reached. However, I do still want to maintain the original check on each format type to ensure the data is a string. According to draft-04 spec,

A format attribute can generally only validate a given set of instance types. If the type of the instance to validate is not in this set, validation for this format attribute and instance SHOULD succeed.

Each format is then specified that it MUST be a string. However, if an attribute is defined as an integer, it shouldn't fail format validation. If I were to ignore checking for a string completely, the regex check would throw a TypeError and this would bubble up to the end user, which is outside of the JSON schema spec.

I updated your request to simply just remove the if !data.is_a?(String) checks, as those are silly to be doing at that point anyhow.

Thanks!

@hoxworth hoxworth merged commit ab2fc8f into voxpupuli:master Jan 4, 2014
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.

None yet

2 participants