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

Added a new field : UniqueField #8

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

AlexTorx
Copy link
Contributor

@AlexTorx AlexTorx commented Aug 2, 2019

This fields aims at ensuring unicity of a field which property 'unique' was set to true, and check this through schema validation.

It has quite the same implementation as the InstanceField but raises ValidationError if the specified value is already used in the given field in the database table.

Tests were written on the same basis as tests for the InstanceField.

@franckbret
Copy link
Contributor

@AlexTorx Looks good, thanks!
@jssuzanne do you see any downside?


class TestSchema(Schema):
uuid = fields.UniqueField(
cls_or_instance_type=fields.List(fields.UUID()),
Copy link
Member

Choose a reason for hiding this comment

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

fields.List ?

@jssuzanne
Copy link
Member

It would be necessary to share the code between the fields which uses a subfield

if not value:
raise ValidationError("Field may not be null.")

if isinstance(self.container, List):
Copy link
Member

Choose a reason for hiding this comment

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

This structure is bizarre, because the notion of fields.List means that the field is multiple whereas it is the value.

it is as if to test the color of the steering wheel of a car, it is considered that a car has several wheels.

personally I am against

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok right I guess I got it. But then since I adapted my code regarding the InstanceField field, why is it different, since this field implements some checks for fields.List cases ?

Copy link
Member

Choose a reason for hiding this comment

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

For me, it is not a list of entry, and it must be only for the post. I think it have not be a field but a validator and only for the serialization.

I will see this evening, If I find a better way

@AlexTorx
Copy link
Contributor Author

AlexTorx commented Aug 4, 2019

It would be necessary to share the code between the fields which uses a subfield

Do you mean create a generical field that should not be instanciated and that contains all methods, and let InstanceField and UniqueField heritate from that new class and re-define the _validate method depending on what the field is actually supposed to validate ?

@jssuzanne
Copy link
Member

Yes, a generic class called "mixin". Python allow multi inheritance dependencies

@AlexTorx
Copy link
Contributor Author

AlexTorx commented Aug 5, 2019

Yes, a generic class called "mixin". Python allow multi inheritance dependencies

Does it look better now ? I have created a "Mixin" class that implements all common methods between InstanceField and UniqueField as well as a dummy _validate method that is supposed to be overloaded.

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