getmoto
Instance variable annotations in moto
LocalStack uses moto as a backend for many emulated services. For these services, we implement the persistence features by serializing and deserializing the various BackendDicts.
Our new implementation of persistence heavily relies on type annotations. For it to properly function, it requires having annotated instance variables for all the Python objects ending up in the BackendDicts.
moto is very rigorous with annotated assignments for BackendDicts. However, the assignments are in the constructor and are not specified as instance variables (e.g., look here for an example).
The same happens for all the other BaseModel classes (e.g., here). In these cases, the assignments are not even annotated.
I would love to propose a small refactoring of the moto models to widely adopt instance variable annotations. This work is clearly beneficial to LocalStack users, but is also in line with modern Python coding standards (e.g., PEP 526). I will open a PR to demonstrate what a refactoring for a service could look like.
I am open to any feedback or comment on these type of changes :)
Hey Gio! Thanks for providing some context around this.
propose a small refactoring of the moto models
Not sure I would classify it as small, considering the scope/size of Moto. :)
Having said that, I completely understand why this is beneficial for LocalStack. But I'm not seeing any benefits for Moto, only some serious concerns:
- the work to add these annotations
- maintaining them, and keeping the annotations in sync with the actual variables used
- explaining to every contributor (many who go through the entire process of checking out a codebase+ learning the rules for just a single bugfix!) why to keep these annotations in sync
- no automated tooling (that I'm aware of) to ensure this is applied correctly
And then there's the tiny detail that this will probably cause a split, where this only applies to the services that LocalStack uses, instead of this becoming a globally consistent style throughout the codebase.
So, all in all, I'm not excited about the idea of introducing this to Moto. I know how important the new persistence framework is to LocalStack, so don't see this as a complete shutdown - but I don't want to approve the attached PR before these concerns are alleviated.
(FYI @bpandola - please let me know if want to add anything/disagree with anything)
I'm with @bblommers on this one. I think his take on the proposal is accurate and fair, so it's gonna be a hard sell for me as well.
hey folks! wanted to chime in here and address some of the (understandable) concerns. we've put some thought into this already before we reached out :-)
Not sure I would classify it as small, considering the scope/size of Moto. :)
true, definitely not small in terms of number of files it will touch 😅 but small in terms of what the actual impact will be.
the work to add these annotations
i understand effort is a concern on your side if you don't see it has meaningful impact for moto. we'd be happy to do the actual work!
maintaining them, and keeping the annotations in sync with the actual variables used explaining to every contributor (many who go through the entire process of checking out a codebase+ learning the rules for just a single bugfix!) why to keep these annotations in sync
we were discussing this as well, and drift is always a risk. to underscore gio's point: we noticed that almost all constructors of models have type annotations. i assume this is also through the proliferation of examples. typically, first-time contributors that try to understand how to do things look at existing code and apply the patterns they can make out. i would imagine moving type annotations from constructors into class declaration would not significantly change that.
so we're not suggesting to further complicate the contribution guidelines here. we believe having consistency in the existing codebase will create a good baseline for contributors to follow organically.
no automated tooling (that I'm aware of) to ensure this is applied correctly
i'm also not aware of any out-of-the-box tooling to do automated verification. but we're not looking for rigorous policing! so same argument as above.
But I'm not seeing any benefits for Moto
i'm not going to pretend we're not doing this for reasons primarily rooted in localstack. that said, i've seen both the moto and localstack codebase evolve to a more standardized pythonic OOP codebase, of which type hints are a key part. we've also seen the benefits of having more comprehensive type annotations for developer experience. it makes error detection and contributions much easier. so i think there is some benefit for moto also, even though not immediately apparent :-)
so what i want to clarify is that we're not looking to change the culture of how moto is developed. that's not up to us! right now, what we're proposing is to refactor the type hints of models into class declarations, which aligns the codebase more with pythonic standards, and at the same time makes reasoning over the models using reflection a bit easier. would that be something you could get on board with?
Hey @thrau ! Hope you're well 😊,
almost all constructors of models have type annotations
That is more because we have mypy enabled across the codebase - if that wouldn't be there, I am fairly certain that the majority of contributors would not add (correct) types. New contributors tend to fail the PR checks exactly because of mypy checks. So that is not an argument for a 'baseline that will be followed organically', unfortunately.
(As an aside.. which constructors do not have types? Everything should have types, so that sounds like something is wrong with our mypysetup 😊)
we've also seen the benefits of having more comprehensive type annotations for developer experience. it makes error detection and contributions much easier
I completely agree with this! But Moto is already completely typed, so we already have these benefits.
The outstanding argument that annotations on class-level is a pythonic standard - I don't think I have enough experience with other codebases to judge that argument. But regardless - just because something is a standard, doesn't mean it's worth doing. And in this case, I don't see any benefits to it I'm afraid.