Skip to content

Conversation

v1ctor
Copy link
Contributor

@v1ctor v1ctor commented Aug 5, 2016

#44

@erosb
Copy link
Contributor

erosb commented Aug 5, 2016

Hello, basically don't mind the idea, but I have a number of concerns regarding the implementation:

  • an other name must be chosen, I find the current one misleading. Maybe "definesProperty".
  • the dot notation of properties is something I'd like to avoid. It would be better to use JSON pointers, since that is a standard notation for the same purpose. It also handles escaping.
  • ObjectSchema#schemaDependencies must be handled, in addition to properties and patternProperties
  • the current implementation of NotSchema#hasField() is confusing. I will think about it
  • Adding a new abstract method to Schema is not a good idea, since it breaks backward-compatibility for people who created their own subclasses (it may be a rare usecase, but still). A default implementation returning "false" would be better, since you could remove most of the overrides.

Sent from my phone, sorry about the typos.
Cheers

@erosb
Copy link
Contributor

erosb commented Aug 6, 2016

@v1ctor are you going to continue working on this?

@v1ctor
Copy link
Contributor Author

v1ctor commented Aug 6, 2016

@erosb yes, sure. I will verify schemaDependencies and change format to pointer notation soon.

@erosb
Copy link
Contributor

erosb commented Aug 6, 2016

Awesome. Please go on with schemaDependencies, and I'd like to do something about NotSchema#definesProperty() too. Do you have any thoughts? Maybe it should always return false.

Please wait a bit with the JSON Pointer notation. The problem there is that we already have some sort of duplication in the library regarding JSON pointers, since

  • we already have a org.everit.json.schema.loader.internal.JSONPointer class, which is not too useful for this task
  • there is also some code for pointer notation handling in ValidationException#escapeFragment()
  • and in the upcoming version of the org.json:json library there is also a JSON Pointer implementation

So I think the best would be to wait for the next release of the org.json lib (coming soon) and use that class for this purpose. I will also try to remove the my half-baked implementations from this schema library.

@v1ctor
Copy link
Contributor Author

v1ctor commented Aug 8, 2016

I've written code to validate schemaDependencies and decided to keep default realisation in NotSchema.

return false;
}

private boolean definesPatternProperty(String field) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can rewrite this method to use the java8 stream API. It isn't a necessary change, but it could be good for practicing it (if you are not familiar yet with streams), due to the simplicity of the task. The same goes for definesSchemaDependencyProperty()

@erosb
Copy link
Contributor

erosb commented Aug 8, 2016

Great job. In the meanwhile the new version of org.json:json is has been published to maven central, but its JSONPointer class doesn't seem to be useful for this scenario. Please go on with changing the dot notation to JSON Pointer notation. The method should accept a JSON pointer in its string representation. Please follow the RFC, and write some tests for verifying the un-escaping of the ~0 and ~1 tokens. Basically you have to do the opposite of what ValidationExcepion#escapeFragment() does. I'm looking forward to merge this PR quite soon, and release 1.4.0 this week.

@v1ctor
Copy link
Contributor Author

v1ctor commented Aug 8, 2016

@erosb Thanks! I've changed realisation to use JSONPointer notation.

@erosb erosb merged commit d4500ca into everit-org:master Aug 9, 2016
@erosb
Copy link
Contributor

erosb commented Aug 9, 2016

All looks fine, I will add some javadoc, then it is ready to be released. Thank you for your contribution.

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