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

QuerySelect and QuerySelectList #84

Merged
merged 8 commits into from Dec 7, 2014

Conversation

Projects
None yet
2 participants
@philtay
Contributor

philtay commented Dec 6, 2014

No description provided.

assert list(field.labels('foo')) == [('bar ' + ch, ch) for ch in 'abc']
assert list(field.labels(str)) == [('bar ' + ch, 'bar ' + ch) for ch in 'abc']
field = fields.QuerySelect(query, 'foo')

This comment has been minimized.

@sloria

sloria Dec 6, 2014

Member

Can you separate the the above assertions from the ones below, e.g. test_query_select_field_func_key and test_query_select_field_string_key?

with pytest.raises(UnmarshallingError):
field.deserialize(['a', 'b', 'b'])
field = fields.QuerySelectList(query, 'foo')

This comment has been minimized.

@sloria

sloria Dec 6, 2014

Member

Same here: separate tests for a callable key from tests for a string key.

with pytest.raises(MarshallingError):
field.serialize('du4', user)
field = fields.QuerySelect(query, 'foo')

This comment has been minimized.

@sloria

sloria Dec 6, 2014

Member

See previous line comment re: test separation.

"""A field that (de)serializes an ORM-mapped object to its primary
(or otherwise unique) key and vice versa. A nonexistent key will
result in a validation error. This field is ORM agnostic.

This comment has been minimized.

@sloria

sloria Dec 6, 2014

Member

Could you please add a brief usage example?

key. In the latter case, the string specifies the name of
an attribute of the ORM-mapped object.
:param str error:
Error message stored upon validation failure.

This comment has been minimized.

@sloria

sloria Dec 6, 2014

Member

For consistency, please also add:

    :param kwargs: The same keyword arguments that :class:`Field` receives.
super(QuerySelect, self).__init__(error=error, **kwargs)
def keys(self):
"""Returns a generator over the valid keys."""

This comment has been minimized.

@sloria

sloria Dec 6, 2014

Member

As per PEP257, docstrings should be in the imperative mode.

The docstring is a phrase ending in a period. It prescribes the function or method's effect as a command ("Do this", "Return that"), not as a description; e.g. don't write "Returns the pathname ...".

https://www.python.org/dev/peps/pep-0257/#one-line-docstrings

class QuerySelect(Field):
"""A field that (de)serializes an ORM-mapped object to its primary
(or otherwise unique) key and vice versa. A nonexistent key will
result in a validation error. This field is ORM agnostic.

This comment has been minimized.

@sloria

sloria Dec 6, 2014

Member

"ORM agnostic" -> "ORM-agnostic"

try:
index = keys.index(val)
except ValueError:
error = getattr(self, 'error', None) or 'Invalid keys.'

This comment has been minimized.

@sloria

sloria Dec 6, 2014

Member

Perhaps the default error message could include the invalid key:

'Invalid key: {0}.'.format(val)
@sloria

This comment has been minimized.

Member

sloria commented Dec 6, 2014

Pass finished. 👍 Just a few nitpicky things to change, and this should be good to merge.

philtay
@philtay

This comment has been minimized.

Contributor

philtay commented Dec 6, 2014

@sloria I can separate the assertions in the unit tests, but this would lead to some (useless) boilerplate duplication. Please let me know if you really want me to do that.
The default error message showing the invalid key is not very feasible because you can't know the full list of invalid keys in QuerySelectList (it immediately raises an exeception in the for loop).
Please also let me know if the tweaks I made to the docstrings in my last commit are ok with you.

@sloria

This comment has been minimized.

Member

sloria commented Dec 6, 2014

In order to reduce boilerplate in the tests, you can define Dummy in tests.base and reuse it where necessary. You could even use a pytest fixture for query if you wanted to.

Even though _serialize and _deserialize fail fast on invalid input, it would still be informative for the user to receive information about which element caused the failure.

On that note, what are your thoughts on building the full list of invalid inputs to QuerySelectList before raising an error? Though this would involve more iteration, the error messages would be more user-friendly.

@sloria

This comment has been minimized.

Member

sloria commented Dec 6, 2014

The docstring changes look good.

@philtay

This comment has been minimized.

Contributor

philtay commented Dec 6, 2014

Ok, I will change the tests as per your suggestion. Regarding the error message, I don't think that wasting (a fair amount of) cycles and memory to generate a default error that almost no one will use and that in any case would be incomprehensible to the final user is a good choice.

philtay added some commits Dec 6, 2014

philtay
@philtay

This comment has been minimized.

Contributor

philtay commented Dec 6, 2014

@sloria It should be fine now.

@sloria sloria added this to the 1.2.0 milestone Dec 7, 2014

@sloria sloria merged commit d89b8f4 into marshmallow-code:dev Dec 7, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@sloria

This comment has been minimized.

Member

sloria commented Dec 7, 2014

👍

@philtay

This comment has been minimized.

Contributor

philtay commented Dec 7, 2014

@sloria my next pull request (tomorrow) will be:

  • 'function' validator (accepts a callable, raises if it returns false, etc etc)
  • 'unique' validator (the most common / wanted validator by those working with ORMs. it will be general and ORM-agnostic of course)
  • 'predicate' validator (just a small improvement, it will raise validation error if the underlying method raises an exception)

Is that ok with you?

@sloria

This comment has been minimized.

Member

sloria commented Dec 7, 2014

In the past, the validate module was meant mostly for internal usage--utility functions for the Email and URL fields to use.

I haven't really considered providing a set of generic user validators. I'm not completely against the idea, but these validators have been implemented many times in other libraries. I'm wondering if there are ways to integrate with other implementations instead of writing them again.

Can you clarify or give an example of how you plan use these validators? Do you create an outer function that calls the functions in the validate module?

# like this?
password = fields.Str(validate=lambda val: validate.length(min=8))

Having to write the outer function seems a bit kludgy, IMO.

@philtay

This comment has been minimized.

Contributor

philtay commented Dec 7, 2014

@sloria

I haven't really considered providing a set of generic user validators. I'm not completely against the idea, but these validators have been implemented many times in other libraries. I'm wondering if there are ways to integrate with other implementations instead of writing them again.

Only in an ideal world unfortunately. In this one, every library raises its own exception and, in general, does things in a slightly different way. Moral of the story: we have to provide at least a minimum set of generic essential validators. For instance, the "function" validator I'm proposing will help with integration. It will raise an exception if the underlying function raises an exception or returns false. You can use it with a validator of another library. On the other hand, we can't ask our users to add another dependency just because we don't provide the "length" validator: the most basic one. Oh, sure, you don't have to add another dependency, you can just write it yourself. Yes, for the millionth time. Then soon after I'll need another basic validator. I'll write it too. Kill me!

Can you clarify or give an example of how you plan use these validators? Do you create an outer function that calls the functions in the validate module?

Here the idea is to convert the functions to classes. Like this:

class Length(object):
    def __init__(self, min=None, max=None, error=None):
        self.min = min
        ...
        ...

    def __call__(self, value):
        """Real validation here."""
        ...
        ...

Of course there are other methods as well, like using functools.partial. I find the class based one the most elegant.
Please let me know which one you prefer and I'll do the work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment