Skip to content

#87 Reference candidate schemas by a descriptive name #108

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

Merged
merged 7 commits into from
Mar 5, 2016
Merged

#87 Reference candidate schemas by a descriptive name #108

merged 7 commits into from
Mar 5, 2016

Conversation

tfesenko
Copy link
Member

@tfesenko tfesenko commented Mar 4, 2016

@tedepstein, the new error message looks like this:
screen shot 2016-03-04 at 4 55 09 pm

I replaced the location by a human readable label and sorted the messages so the best candidate (least amount of errors) is displayed first.

@tedepstein
Copy link
Collaborator

Oh, @tfesenko , that is so much better!!!

@tfesenko
Copy link
Member Author

tfesenko commented Mar 4, 2016

@tedepstein , and no changes to the Swagger Schema!
I can create more screenshots for you then.

return description.asText();
}
// "$ref":"#/definitions/headerParameterSubSchema"
JsonNode ref = swaggerSchemaNode.get("$ref");
Copy link
Collaborator

Choose a reason for hiding this comment

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

We might have to be careful with Refs. I remember seeing in a discussion that JSON Refs can use JSON Pointer as the locator, but they don't have to. I think they may use a different form when it's an external reference. Your algo may still work here, but it's a good idea to check on the JSON Ref specification.

@tedepstein
Copy link
Collaborator

Passed code review.

@tfesenko
Copy link
Member Author

tfesenko commented Mar 4, 2016

@tedepstein , a screenshot with the title you've just provided:
screen shot 2016-03-04 at 6 26 26 pm

@tedepstein
Copy link
Collaborator

@tfesenko , I would suggest adding a couple of unit tests to prevent future failures:

  1. Verify that all $ref: values in the schema yield a non-empty string when processed with your name extractor function (substring after the last / slash character).
  2. Verify that all allOf or anyOf candidate subschemas are $refs (already verified in test 1) or have a "title" property.

@tedepstein
Copy link
Collaborator

Test results:

  • Logged an issue re: unsigned code warning
  • Invalid parameter test passed: image
  • additionalProperties test passed:
    image
  • array/items test passed:
    image
  • format test passed, sort of. It didn't list the two alternatives (user-defined format string vs. pre-defined enum format string). Maybe because the validator combines the type validation on these two cases, and fails generally on an invalid type. In any case, I am happy enough with this error message:
    image

@tedepstein
Copy link
Collaborator

Passed QA. Merging.

tedepstein added a commit that referenced this pull request Mar 5, 2016
#87 Reference candidate schemas by a descriptive name
@tedepstein tedepstein merged commit f25c482 into RepreZen:master Mar 5, 2016
@tedepstein tedepstein deleted the task/87 branch March 5, 2016 00:04
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.

2 participants