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

Rename Schema to Field? #577

Closed
samuelcolvin opened this issue Jun 5, 2019 · 13 comments · Fixed by #824
Closed

Rename Schema to Field? #577

samuelcolvin opened this issue Jun 5, 2019 · 13 comments · Fixed by #824

Comments

@samuelcolvin
Copy link
Owner

@samuelcolvin samuelcolvin commented Jun 5, 2019

Currently we have the class Schema for when you need to define more stuff on a field than just the type and default.

But this is a confusing name since not all it's arguments refer to the schema. It'll get more confusing if we want to add more arguments to Schema/Field, eg. a more complex alias system #477.

Therefore we should rename it to Field.

This would require:

  • renaming the current Field class, perhaps renaming the entire fields.py module.
  • Schema needs to carry on working for a while but with a deprecation warning

What does everyone thing?

@tiangolo Schema was your work originally, are you ok with this?

@samuelcolvin samuelcolvin mentioned this issue Jun 5, 2019
3 of 5 tasks complete
@samuelcolvin samuelcolvin added this to the Version 1 milestone Jun 5, 2019
@tiangolo

This comment has been minimized.

Copy link
Collaborator

@tiangolo tiangolo commented Jun 5, 2019

Thanks for tagging me. Yep. I think it makes sense.

If we can keep supporting it for a while with a deprecation warning, we should be fine.

And thinking about it, it actually makes more sense for it to be Field.


Slightly related, currently Schema (soon to be Field) is a class. When declaring a Pydantic field with:

a: int = Schema(...)

mypy complains that a should be of type int but is being of type Schema.

One way I'm "hacking" it in FastAPI (for stuff that inherits from Schema) is to create a function with a return type annotation of Any (while it is returning a Schema instance).

Then because the return of the function is annotated as Any, mypy accepts it.

I think this is something that could be interesting to have here too, maybe worth considering during this change.

@jasonkuhrt

This comment has been minimized.

Copy link
Contributor

@jasonkuhrt jasonkuhrt commented Jun 11, 2019

In favour of this change too.

This seems to align better with dataclass terminology too, https://docs.python.org/3/library/dataclasses.html#dataclasses.field.

For consideration: is there a possibility that the similarity would be a bad thing, rather than good?

@tiangolo

This comment has been minimized.

Copy link
Collaborator

@tiangolo tiangolo commented Aug 5, 2019

I agree with @jasonkuhrt that using a field function would be nice. Being a function that actually returns a class would fix mypy errors when using Schema (or the next, Field), as a function can be typed to return Any. In fact, I'm doing something like that in FastAPI.

Also, by implementing a field function right away we could update the docs and show the new interface to new users, even while Schema is not yet renamed to Field (and the current Field to something else).

I wanna take a shot at this.

I have some naming questions:

  • Is it OK a field function exposed as the main interface instead of the corresponding class?
    • Would you prefer another function name? Or a capitalized version (a function Field) that simulates the class name, even while it's a function?
  • What would be an acceptable name for the current Field class?

On the other side, having just a function field that returns a Schema class would solve the visible naming issue for users, that could be enough, without needing to rename the underlying classes. The drawback is that the internal names would be more difficult for newcomer contributors to the internal code.

@samuelcolvin

This comment has been minimized.

Copy link
Owner Author

@samuelcolvin samuelcolvin commented Aug 5, 2019

I think the function should be called Field, it should have an alias called Schema that also returns a Field but also raises a warning.

The only question is what do we call the class? It would be confusing to also call it Field, how about FieldInfo?

Please take a shot. I want to work on v1 features once v0.32 is released.

@tiangolo

This comment has been minimized.

Copy link
Collaborator

@tiangolo tiangolo commented Aug 5, 2019

Perfect.

So, the current Schema class will be called FieldInfo?

And how should the current Field class be called? FieldAttr, FieldObject, FieldElement, Attr, (something else). Or should we keep it just Field?


In summary, the next parts will be:

  • A Field function that returns a FieldInfo class instance.
  • A Schema function that returns a FieldInfo class instance and logs a warning.
  • A FieldInfo class that will replace the current Schema class.
  • A (what?) class that will replace the current Field class (or do we just keep this Field class)?
@samuelcolvin

This comment has been minimized.

Copy link
Owner Author

@samuelcolvin samuelcolvin commented Aug 6, 2019

Looks good, I think current field becomes Attribute, we should also rename that file.

@samuelcolvin

This comment has been minimized.

Copy link
Owner Author

@samuelcolvin samuelcolvin commented Aug 6, 2019

Correction, Attribute is a dumb name since then you would have model.__fields__: Dict[str, Attribute] which is confusing.

Better to rename what used to be called Field as ModelField?

@dmontagu

This comment has been minimized.

Copy link
Collaborator

@dmontagu dmontagu commented Aug 6, 2019

+1 for ModelField, FieldInfo, and Field function that returns a FieldInfo

@skewty

This comment has been minimized.

Copy link
Contributor

@skewty skewty commented Sep 3, 2019

This may be controversial, but the library attrs supported typing (before python 3.6) using a type parameter. Field could have two positional arguments (type and default) then the Field function could actually return the correct type (using a Generic).

class Foo(BaseModel):
  maximum = Field(int, 100)

# this would require a different code path
class Bar(BaseModel):
  maximum: int

# this would be bad but possible
class Foo(BaseModel):
  maximum: int = Field(str, "100")

Would having the type as an arg to the Field method provide any additional benefits?

@dmontagu

This comment has been minimized.

Copy link
Collaborator

@dmontagu dmontagu commented Sep 3, 2019

I see a variety of approaches to the possible signature of the Field function; I've included my breakdown below. (I use FieldType to refer to the type of the object actually returned by Field.)

I'm most in favor of one of the first two approaches, and would be fine with the first one (the current proposal).

  • Proposed implementation: def Field(default: Any, **etc) -> Any
    • (All pros/cons below are relative to this implementation)
  • The following overloaded approach (which may require some minor tweaks)
    @overload
    def Field(default: T, **etc) -> T: ...
    @overload
    def Field(default: None, **etc) -> Any: ...
    @overload
    def Field(default: Ellipsis, **etc) -> Any: ...
    def Field(default: Any, **etc) -> Any: ...
    • Pro: type-checks default value against the annotated type where possible
    • Con: Incorrect type hint if you actually want to treat the result as a FieldType
    • Conclusion: currently my favorite alternative to the proposed implementation; depends on whether we might ever want to work with the returned value of the Field function as a FieldType.
  • def Field(type_: Type[T], default: T, **etc) -> T
    • Pro: type-checks the default value against the specified type
    • Con: Incorrect type hint if you actually want to treat the result as a FieldType
    • Con: Requires you to repeat the type if you want to use an annotation. If you don't use an annotation, then the fields may occur out of order, potentially leading to confusing validation errors.
    • Conclusion: Given the second con, I would probably prefer the prior approach.
  • def Field(type_: Type[T], default: T, **etc) -> Any
    • Pro: type-checks the default value against the specified type
    • Con: Even more so than the previous example, encourages you to drop the annotation to prevent them from getting out of sync (since there is no check they are equal)
    • Conclusion: given the field-ordering issue described above, I would probably prefer the overload-heavy implementation (despite getting an incorrect type hint for FieldType)
  • Approaches where FieldType is generic and Field returns a FieldType[T] (this is actually what attrs does, though the library mechanics are sufficiently different to cause different issues)
    • E.g.:
      • def Field(default: T, **etc) -> FieldType[T]
      • def Field(default: T, type_: Type[T], **etc) -> FieldType[T]
    • Pro: provides the type checker with the most accurate information
    • Con: will cause mypy errors when used to annotate fields in the absence of a mypy plugin; with a mypy plugin, this is probably unnecessary anyway.
    • Conclusion: Probably not even an improvement over the current implementation

@skewty do those points change your thinking at all?

@skewty

This comment has been minimized.

Copy link
Contributor

@skewty skewty commented Sep 4, 2019

@dmontagu I hadn't mentally considered "Con: Incorrect type hint if you actually want to treat the result as a FieldType". Thank you.

Given the above, I would choose method 1.

@dmontagu

This comment has been minimized.

Copy link
Collaborator

@dmontagu dmontagu commented Sep 4, 2019

@skewty for what it’s worth, I’m not sure it’s actually that big of a downside for the following reasons:

  1. You can always use the FieldType class directly
  2. Internally, there is little reason to use the Field function anyway
  3. Externally, there is little reason to need to interact with the result of Field as a FieldType anyway
  4. Type hinting as Any prevents any type safety benefits anyway, so if Field is going to be rarely/never used to produce something interacted with as a FieldType, it might make sense to just use # type: ignore in the few places you use for that purpose anyway.
@dmontagu

This comment has been minimized.

Copy link
Collaborator

@dmontagu dmontagu commented Sep 26, 2019

I created a separate issue for this, but there may be another alternative: typing_extensions.Annotated (now supported by mypy). See #837 for more detail; this would allow us to remove the Field function that returns a FieldType but is annotated as returning Any.

@tiangolo this could also be useful to support in fastapi (where I could grab the Depends from the associated Annotated data, rather than assigning a default value).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.