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

Public API to check a declared attribute type and to check if it is mandatory/optional #159

Closed
wants to merge 3 commits into from

Conversation

smarie
Copy link
Contributor

@smarie smarie commented Mar 2, 2017

Hello everyone,

First of all thanks a lot for this great library ! I am sad I found it only a couple days ago, otherwise I would probably not have spent time writing classtools-autocode. Indeed its main idea was to remove some of the boilerplate and link with PyContracts for validation.

Anyway, today I make this pull request because in another project of mine currently in development mode, parsyfiles, I perform some type introspection to understand what is required to parse an object. I use PEP484 type hints for that, but I wanted to support attrs too so I implemented something that does the job, and now I am proposing it to you.

The proposal is made of two functions : 'guess_type_from_validators', and 'is_mandatory'. Feel free to change the naming, I am not particularly good at finding great names.

For 'is_mandatory': it is simply checking if the validator is an instance of 'optional'.

For 'guess_type_from_validators': type information is extracted from the 'instance_of' validator. In order to help users rely on this validator instead of doing the same type checking in their custom validators, I introduce the 'chain' validator. Note that this might now be useless, since I saw in the source that you allow lists of validators.
In the future if issue 97 is resolved, the 'guess_type_from_validators' method should be updated to also use the new python 3.6 type hints as an additional source of type hint.

I added some unit tests for all the code, and updated the doc in api.rst.

Let me know what you think.

…file-collection-parsing-framework):

* added new 'chain' validator to allow users to chain their custom validators with, for example, 'instance_of'
* added utility methods to get the declared attribute type (by inspecting its validators) and to check if it is optional
…file-collection-parsing-framework):

* added tests
* fixed bug with optional validator to pass tests :)
@hynek
Copy link
Member

hynek commented Mar 4, 2017

So the good news is that chain is not necessary anymore: as of the (unreleased) 17.1, you can pass a list of validators and it just works.

guess_type_from_validators is certainly very useful but I’m afraid a bit too hacky to be part of an official API (I do encourage anyone to release their own validators and other attrs-related tools tho! I just try to keep attrs slim and opinionated. I prefer no API in core over an API that contains the word guess :)).

is_mandatory opens two interesting questions:

  1. optional() should be able to consume lists like validator= does. (filed as validators.optional should take lists like validator= #161)
  2. where to put it? It’s more introspection than a validator…

@smarie
Copy link
Contributor Author

smarie commented Mar 7, 2017

Thanks for the quick review @hynek,
Agreed that you can get rid of chain.. if optional() is able to consume lists.
I will keep my 'hacky guess' method in the parsyfiles project, no problem about it not being part of the public attr API - indeed that was borderline.

@Tinche
Copy link
Member

Tinche commented Mar 7, 2017

Pulling metadata from validators is something I've played around with and ultimately I think is just too hacky. It won't work with other validators, what about using converters instead of validators, etc.

@smarie I would advise using metadata (the proper, attrs metadata available on Attribute instances).

@hynek
Copy link
Member

hynek commented Mar 24, 2017

Yeah so what Tin said. Thanks for sharing your work but this is nothing for attrs proper.

@hynek hynek closed this Mar 24, 2017
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.

3 participants