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

Add handlers to validate methode and introduce format_checkers #57

Merged
merged 2 commits into from
Sep 24, 2019

Conversation

ystreibel
Copy link
Contributor

Hello,

We would like to contribute in this project by adding the format_checkers feature that consists to allow custom format to be check during the compile method execution.
The format_checkers is optional and only available for compile.

A fellow, @gpakosz, already contact you about this.

Thanks

@gpakosz
Copy link

gpakosz commented Jul 29, 2019

Hello @horejsek 👋

As discussed by email, we thought it makes sense to augment validate() and compile() with a format_checkers dict that maps a format name to a callable. The current implementation assumes nothing of the callables:

  • they can be stateful
  • they can raise anything (which will be propagated with Python 3 exception chaining)

This is a feature we need to move from python-jsonschema which is too slow for our current workload.

@horejsek
Copy link
Owner

horejsek commented Aug 6, 2019

Thank you for pull request.

I would like to see tests for that. Other improvement I think could be to do custom formats first to be able to override formats from the JSON spec. Could we also call it simply formats and have there callbacks or regular expressions? If the check can be only regular expression, then it can be much faster than callbacks.

@gpakosz
Copy link

gpakosz commented Aug 7, 2019

Hello @horejsek,

The current implementation indeed doesn't replace the existing format checking that's regex based. Maybe we should just do that instead of running after the default ones.

Agreed with naming the dict formats and make it either a callback or a regular expression. What types do you think should be supported as values?

  • str, re.Pattern, function?
  • or just re.Pattern and function?

@horejsek
Copy link
Owner

horejsek commented Aug 7, 2019

Because often regular expression is written as r"...", I would allow str being interpreted as regular expression as well.

@ystreibel
Copy link
Contributor Author

Hello, I took your requests into consideration.

Now, I would need more information about testing! I would like to know where could I add my tests.
It seems to be in template_test function in utils.py and add it in the compile but if I do that I couldn't test all the formats types (str, re.Pattern, function) but just one.

Thank you.

@ystreibel
Copy link
Contributor Author

Hello @horejsek 👋,

Did you see my previous comment? We really need to know how to improve the test suits to finalize this PR.

Thank you

@horejsek
Copy link
Owner

Sorry, I have a lot of things going on last two month and couldn't pay attention to this issue.

utils.py in json_schema folder is meant for test suits from JSON Schema. Do not use this one for this. Write your own test in tests folder. Because you are changing how format feature is working, there is file tests/test_format.py for that. You can see usage of asserter which is defined in conftest.py, see: https://github.com/horejsek/python-fastjsonschema/blob/master/tests/conftest.py#L17 You can add parameter (not required) which will also configure format checkers and then you can write tests similar to ones already in test_format.py.

@ystreibel
Copy link
Contributor Author

Hi @horejsek ,

I added some tests as you explained me. You could finally check the PR.
Thank you for this help.

@horejsek horejsek merged commit aa4ea89 into horejsek:master Sep 24, 2019
@horejsek
Copy link
Owner

Those tests are specific to your custom formats, this library needs tests which cover the functionality itself. I will do it. Also, re.Pattern is there since Python 3.7 and I officially still support 3.3. That's maybe old, but at least 3.5 is needed. I will also fix that.

@horejsek
Copy link
Owner

BTW your tests actually do not work at all...

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

3 participants