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

There's no class IValidator in your package and we know nothing about defaults. #239

Closed
eirnym opened this issue Jul 1, 2015 · 13 comments
Closed

Comments

@eirnym
Copy link

eirnym commented Jul 1, 2015

Documentation massively links on jsonscheme.IValidator class which is not present in code, also base class of validator is hidden within factory. So I can't create my own custom validator.

Also I have no information about defaults without monkey patching

To extend array type with tuple(#148), we have toto add code like this:

def _add_tuple(types):
    result = dict(types)
    result['array'] = (list, tuple)

class Validator(Draft4Validator):
    DEFAULT_TYPES = _add_tuple(Draft4Validator. DEFAULT_TYPES)
@Julian
Copy link
Member

Julian commented Jul 1, 2015

Yes, it's true, there is no such class, it's an informal interface that validators follow. This is slightly confusing, and at some point there was some effort towards adding a zope.interface, but it was dropped because there wasn't any concrete benefits we needed to add it.

I don't know what you mean about having no information about defaults. What information do you want, and what defaults are you talking about. Again, please be very specific.

What you have there is definitely not necessary as the exact issue you linked to already says. Validators take a types dictionary which lets you specify what the types mapping is. That's documented too, please read it, and ask specific questions.

And yes, the validator base class is "hidden". Inheritance is not a supported extension API for jsonschema, this is intentional. There are other, better APIs for extending existing validators. Those are also documented.

Throughout, as I mentioned, specific issues with the documentation would definitely be appreciated, as pull requests if you can provide fixes, or issues if you cannot.

Thanks again.

@eirnym
Copy link
Author

eirnym commented Jul 1, 2015

Please, add validator using ABCMeta and abstractmethod decorator.

@Julian
Copy link
Member

Julian commented Jul 1, 2015

I don't have plans to use ABCs, they are a misfeature. What exactly are the things you are looking for here?

@eirnym
Copy link
Author

eirnym commented Jul 1, 2015

Contstants, methods, docstrings, better subclassing.

@eirnym
Copy link
Author

eirnym commented Jul 1, 2015

and no monkey patching for class customising

@Julian
Copy link
Member

Julian commented Jul 1, 2015

Subclassing is not supported intentionally, nor is monkey patching obviously.

What do you mean by "constants" and "methods"?

@Julian
Copy link
Member

Julian commented Jul 1, 2015

Or docstrings, which all of the classes have.

@eirnym
Copy link
Author

eirnym commented Jul 1, 2015

Why is subclassing not supposed? For example reasons, user doesn't want to wait when draft/release will be implemented or can't update library for some reasons like hard dependency link.

ABCMeta gives information about which methods are exists, docstrings, but nothing about implementation. And this is not too bad.

@Julian
Copy link
Member

Julian commented Jul 1, 2015

Subclassing is not supported because inheritance is bad, and inheritance is not an appropriate tool to use for creating public APIs. What you mentioned is perfectly possible. The public APIs for creating validators are not subclassing. It's jsonschema.validators.create to create one from scratch, and jsonschea.validators.extend to extend from existing ones. These allow you to create new validator classes for new drafts, or custom drafts, or whatever else you want. The only difference is how you use it, it's not class MyDraft5Validator(Draft4Validator), or anything of that sort, it's an explicit, designed, supposedly-fully-featured API (but feel free to bring up things that are missing).

To your second point, yes, that is all true, ABCMeta does. So does the documentation? It specifies all the methods you have available, and what they do. What additional benefits do you want out of ABCMeta?

@eirnym
Copy link
Author

eirnym commented Jul 1, 2015

@Julian I understand that class MyDraft5Validator(Draft4Validator) is bad thing, but if I'll have full public API, I can implement any custom validator I want to. Also I'll prefer separation like: Validator TypeChecker, FormatChecker (Draft4TypeChecker, Draft4FormatChecker, etc)

In most cases Validator will be not altered by subclasses.

Code may look like:

class Validator(object):
    # None values are for Python2 compatibility only
    def __init__(name, *args, type_checker=None, format_checker=None, **kwargs):
        self.type_checker = type_checker()
        self.format_checker = format_checker()
        ...   # rest of common validator code

class Draft4Validator(Validator):
    def __init__(*args, type_checker=Draft4TypeChecker, 
                 format_checker=Draft4FormatChecker, **kwargs):
        # Python3-like super()
        super().__init__('Draft4Validator', type_checker=type_checker,
                         format_checker=format_checker, *args, **kwargs)   
       # rest of custom validator code if applicable.
...

validator = Draft4Validator()
validator.validate(payload)

Current validator and most of public API doesn't have any inline documentation at all. It's really bad.

@Julian
Copy link
Member

Julian commented Jul 1, 2015

You have a full public API and you can implement any custom validator you want to. There is no TypeChecker because there is only 1 method related to type checking, and every validator will need to do it. Instead there's an API for interacting with it, the types dict.

You're still not being concrete. What is a thing you are prevented from doing with the current API, and what documentation is missing?

@Julian
Copy link
Member

Julian commented Jul 1, 2015

The entire public API should have docstrings. Feel free to point out an example where that isn't the case and it can be addressed. Patches are also welcome.

@Julian
Copy link
Member

Julian commented Jul 27, 2015

Closing this but feel free to follow up if you have any questions, would love to help.

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

No branches or pull requests

2 participants