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

consider a function-layer in the ORM that returns "Any" #174

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

zzzeek
Copy link
Member

@zzzeek zzzeek commented Sep 9, 2021

all ORM functions that are used in declarative are now
made separate from the classes they refer towards in all
cases and indicate the return type of Any so they can be
used inline with Mapped[_T] in declarative mappings
such that IDEs can deliver Mapped[] behavior without any
linter errors.

this will require changes in the mypy plugin as we
can no longer rely on simple things like "composite()" returning
CompositeProperty.

References: https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/3074

Fixes: #170

Description

Checklist

This pull request is:

  • A documentation / typographical error fix
    • Good to go, no issue or tests are needed
  • A short code fix
    • please include the issue number, and create an issue if none exists, which
      must include a complete example of the issue. one line code fixes without an
      issue and demonstration will not be accepted.
    • Please include: Fixes: #<issue number> in the commit message
    • please include tests. one line code fixes without tests will not be accepted.
  • A new feature implementation
    • please include the issue number, and create an issue if none exists, which must
      include a complete example of how the feature would look.
    • Please include: Fixes: #<issue number> in the commit message
    • please include tests.

Have a nice day!

@zzzeek zzzeek requested a review from CaselIT September 9, 2021 21:18
) -> Any: ...
@overload
def mapped_column(
__type: Union[schema._TE, Type[schema._TE]],

Choose a reason for hiding this comment

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

Would it be possible to make __type: TypeEngine[X] and then make the function return Mapped[X], so that mapped_column(Integer) -> Mapped[int] automatically, or something similar ?

Copy link
Member Author

Choose a reason for hiding this comment

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

not sure. my understanding so far has been that pep484 can't propagate a nested type like that.

the things i am thinking about are:

  1. Column and all other ORM constructs being treated the same
  2. whether to create a new suite of functions altogether
  3. starting to introduce mapping straight from annotations

number 3 above is what you are interested in. the idea is this would be a submarine kind of feature in 1.4 since we are deep into it, then in 2.0 it would be more formally introduced, which is:

@reg.mapped
class MyClass:
    __tablename__ = 'foo'

   id: Mapped[int] = mapped_column(primary_key=True)
   name: Mapped[str]
   fixed_name: Mapped[str] = mapped_column(String(50))
   other_data: Mapped[datetime] = mapped_column(nullable=False)
   binary_data: Mapped[bytes] = mapped_column(mysql.MEDIUMBLOB)
   related: Mapped[List["Related"]] = relationship()

   

it should be apparent what we're doing above which is semi-ripping off dataclasses, and adding to declarative the ability to link the annotated types to a pre-fab typeengine. one challenge is that MySQL doesn't allow VARCHAR without a length so we'd have to introduce some "max length" construct to that dialect and maybe others.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK I think having it return Mapped[_TE] works? at least the IDE is cool with it lets see what mypy says

Copy link
Member Author

Choose a reason for hiding this comment

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

oh wait, it doesn't. not exactly. IMO I thnk pep484 should make this possible but im not sure that it does, there's likely some comp sci name for what this pattern is

Copy link
Member Author

Choose a reason for hiding this comment

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

ok it does. im not sure why things werent done this way w/ the stubs, but I guess it's because of the mismatch with Column() etc. OK redefined functions that return someting new might be how to go w/this

all day today ive been looking at how we probably dont need the mypy plugin for most things except the constructor

Screenshot from 2021-09-10 16-09-09

Copy link
Member

Choose a reason for hiding this comment

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

pylance seems to work a lot better than mypy is this a thing?

it does not seem a high bar to pass

Choose a reason for hiding this comment

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

in my experience pylance works faster and better than mypy, of course it's all subjective :)

Choose a reason for hiding this comment

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

You can command palette-> Reload window to 'soft-restart' vscode, it'll reload the extensions. Not sure if there's a specific way to just reload stubs.

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Regarding the Optional thing, it's true that just after creating a new object, the fields may be None if they weren't passed a value in the generated init, however in the more general case, when receiving an object from the database, one can assume that if the column is not nullable, there will be some kind of value. Ideally the init would make the non-nullable fields mandatory, but that would be quite a breaking change. Practically speaking it would be annoying if every usage of an object field that is non-nullable, would be squiggly because maybe it's None, this would generate a lot of noise and would lead to turning the thing off I suspect.

_type: Union[sqltypes.TypeEngine[_T], Type[sqltypes.TypeEngine[_T]]],
*args: SchemaEventTarget,
autoincrement: Union[bool, Literal["auto", "ignore_fk"]] = ...,
default: Optional[Any] = ...,

Choose a reason for hiding this comment

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

Can this be Optional[T] ?

Choose a reason for hiding this comment

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

Maybe this isn't clear from the github interface, but I meant "default: Optional[T] = ..."

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.

Supporting __get__ and __set__ on Column object
3 participants