-
-
Notifications
You must be signed in to change notification settings - Fork 66
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
my 2 cents #61
my 2 cents #61
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for yours 2 cents. I didn't check the whole pull request. Unfortunately, you are working on the same issues I do and I have different view how to solve them. I like some ideas, though, and I would like to accept them. When I will be done, I will let you know.
# pylint: disable=redefined-builtin,dangerous-default-value,exec-used | ||
def compile(definition, handlers={}, formats={}): | ||
# pylint: disable=redefined-builtin,exec-used | ||
def compile(definition, handlers=None, formats=None, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have dicts there on purpose. With them in documentation is clear the value should be a dict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since I started refactoring your library for my own purpose and I this is my first contribution ever to open-source, I stack to the coding standards/linting we use. Since we all agree that there's only python3 left - we can leave the docs to annotations. But if you prefer mutable defaults, it is a choice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, Python 2 support is dropped so we could use annotations instead.
validate = fastjsonschema.compile(definition, formats={ | ||
'foo': r'foo|bar', | ||
'bar': lambda value: value in ('foo', 'bar'), | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you remove the doc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, this must have skipped my attention somehow.
return resolver, code_generator | ||
|
||
|
||
def _get_code_generator_class(schema): | ||
def get_code_generator_class(schema): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to make this function "public".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clear. With the added format handlers I think they can stay private. The point is that we used the version 2.12 and there I had to duplicate the code so I could monkey patch the generator class.
@@ -28,17 +46,20 @@ class CodeGenerator: | |||
|
|||
INDENT = 4 # spaces | |||
|
|||
def __init__(self, definition, resolver=None): | |||
def __init__(self, definition, resolver=None, formats=None, verbose=False, root_name='data'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have better plan for exceptions. I want to enhance exception class with more information which is possible to parse with a code. So no need to verbose. If you will wait, you will see. Unfortunately you started on it when I already have something in progress.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, i could not wait and hope that stuff will get better in the repo, so i went on my own and was finished and then realized that maybe I could give back some of my changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand.
with self.l('def {}(data):', name): | ||
self.generate_func_code_block(definition, 'data', 'data', clear_variables=True) | ||
self.code_break() | ||
with self.l('def {{}}(data, scope="{}"):'.format(self._root_name), name): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get the reason for root_name...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when you have a lot of json schema files e.g. article.json product.json etc, our api had to come up with better error messages that data.teaser[0] must be ....
, so now when compiling article.json with root_name='article' the message is article.teaser[0] must be ...
the default value for scope for a function block is root_name which should be 'data' as in 2.12 for compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So your API returns in the response the message from this library? I think this can be improved with my idea of more information on the exception class, so your API can use it and generate nice messages which matches the other messages in your app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you want (as i read in feature requests) to add attributes to the exception - that's great. for me the most important feature is to keep track of the context/scope of where the validation process is at when the exception is thrown. so given: a schema:
article.json
"$schema": "http://json-schema.org/draft-07/schema#",
"type" : "object",
"additionalProperties": false,
"properties": {
"teaser" : { "$ref": "intro.json" },
"prolog" : { "$ref": "intro.json" }
}
}```
when the error occurs during the prolog validation, the exception contains an attribute with value `article.prolog.....` about where it happened (like the jsonschema library does).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I will make issue for that. Or do you want to do it on top of latest master?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, i suppose now i will have to wait for you to finish your ideas and then try to somehow keep the best of two. as i said, basically i was pretty much content with the refactor and now it is part of the production code. my pull request was merely a try to give back some of my effort for better user experience of your library
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have my changes released. I created issue for your needs. If you want, you can upgrade your version of fastjsonschema, apply that feature on top of it and do a new merge request, or I can do it. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I would like to contribute if you are interested but as you pointed out your vision of where to go differs from what I have proposed, so perhaps working collaboratively might be a better way than keep merging two ideas all the way. Let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can check out the exceptions--they have more information which can help apps to generate better message for the user. Another step is to be able to return all errors at once. Your improvement is also good, to remember the whole path in connected definitions. If you want to implement this on top of the master, go ahead. If you want to keep your version of JSON schema, it's ok, I can implement it here one day.
@@ -1 +1 @@ | |||
VERSION = '2.13' | |||
VERSION = '2.12' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accident?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, as i mentioned, i forked off version 2.12 then rebased the current master onto my repo
|
||
# pylint: disable=invalid-name | ||
@indent | ||
def l(self, line, *args, **kwds): | ||
def l(self, line, *args, deduplicate=False, expand_locals=True, **kwds): # noqa |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This library is using pylint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct. can be removed. (as i mentioned, the library code lives in the project and i had to lint it with existing tools. when the pull request gets merged, i can happily go using the official version)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. This merge request is actually a patch of changes you did for your copy of this library in your app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exactly
if self._verbose: | ||
message += '\\n caused by {path} {rule}' | ||
kwargs.update(path=self.context_path(), rule=self._definition) | ||
self.l('raise JsonSchemaException("{}")'.format(message if expand else ''), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there some real performance gain with this probing? If not, I think it's then uselessly complex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not really, but when the the _definition is huge... it is huge in the compiled schema
I really much appreciate the idea and code of this library. In our project we have 20 schemas linked via ref and so running a validation against invalid data gives you a puzzle work to spot the problem (whereas jsonschema library shows the problem directly). I wanted to have a better message so I came up with this patch, which among others contains:
required
test, only show properties missing