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

Treatment for many and only parameters #731

Conversation

4lissonsilveira
Copy link
Contributor

@4lissonsilveira 4lissonsilveira commented Jan 30, 2018

  • A small treatment to avoid conflict with only and exclude params
    has been created.
  • An If has been created to verify if many parameter is boolean.

Related: #406

- A small treatment to avoid conflit with only and exclude params
has been created.
- An If has been created to verify if many parameter is boolean.
@4lissonsilveira
Copy link
Contributor Author

@sloria do you think that is a good approach? Could you(or another contributor) give me any suggestions for improvements?

Thanks!

@@ -386,6 +386,22 @@ def __init__(self, nested, default=missing_, exclude=tuple(), only=None, **kwarg
self.many = kwargs.get('many', False)
self.__schema = None # Cached Schema instance
self.__updated_fields = False

if not isinstance(self.many, bool):
raise ValueError('Many parameter must be boolean')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

many does not have to be a boolean. You can pass anything, as long as it evaluates to a either truthy value if you expect a collection or a falsy value if you expect a single element.


# Catch fields used at the same time
# into only and exclude parameters
fields = [item for item in only if item in list(exclude)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use set intersections to do that.

@lafrech
Copy link
Member

lafrech commented Jan 30, 2018

I'm not sure we're willing to add this kind of validation.

  • You may want to pass anything as many. It does not have to be a boolean. It will evaluate to True or False.

  • I believe only has priority over exclude (or conversely, I don't remember). Raising an exception when a field is in both lists may break some workflows. However, I agree that relying on this may be a bit scary, and in some cases, it could hide an error. And the fact that I can't remember which one takes priority and I can't find it quickly in the docs makes me think it could be sensible to prevent it. I suppose the assumption is "Since it can be solved by an explicit priority and it creates no issue, why prevent it? The user should be aware or what he is doing." Same applies to many.

@deckar01
Copy link
Member

These seem like good warnings for new users, but would likely be burdensome errors for existing users.

@sloria
Copy link
Member

sloria commented Jun 11, 2018

Closing this in favor of #826

@sloria sloria closed this Jun 11, 2018
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

4 participants