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

Let IP fields inherit String #2250

Closed
wants to merge 1 commit into from
Closed

Let IP fields inherit String #2250

wants to merge 1 commit into from

Conversation

lafrech
Copy link
Member

@lafrech lafrech commented Mar 15, 2024

Any reason not to inherit String here?

Due to this, in apispec, the type wouldn't be set to string. This is being addressed there by setting type to string and the proper format, but I suppose it makes sense to "fix" this here as well.

@sloria
Copy link
Member

sloria commented Mar 19, 2024

i'm unsure about this. we purposely decided to inherit from Field instead of String: 62fdfa8 . see this comment: #1485 (comment)

As I can see, fields inheriting from String, are in general deserialized from strings to strings with additional input value format validation. We can see it in the Url and Email fields cases. Exception from this rule is the UUID field, which deserializes from string into UUID object representation. Therefore, it is not clear for me why UUID inherits from String and not Field, like in the case of DateTime or TimeDelta which also deserializes to object representation.

@lafrech
Copy link
Member Author

lafrech commented Mar 19, 2024

I guess I get the rationale. The type expressed in the field name (and its parents in the class hierarchy) represents the type in object world, not in serialized world.

Alright, I'm fine with this. This came from apispec where the string type must then be specified in the field class -> JSON type mapping, but that's not a blocker.

Thanks for digging into this.

@lafrech lafrech closed this Mar 19, 2024
@lafrech lafrech deleted the ip_field_string branch March 19, 2024 20:52
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.

2 participants