This repository has been archived by the owner on May 17, 2024. It is now read-only.
Convert to attrs, remove runtype
#723
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Oct 2, 2023
nolar
force-pushed
the
fix-aftermath-of-refactoring
branch
from
6be7a94
to
902fcc6
Compare
nolar
force-pushed
the
attrs-instead-of-runtype
branch
from
1d40c2b
to
59d9196
Compare
nolar
force-pushed
the
attrs-instead-of-runtype
branch
from
59d9196
to
33deadb
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
nolar
force-pushed
the
attrs-instead-of-runtype
branch
from
33deadb
to
6386699
Compare
Always, always, always call the inherited constructor — even if it does nothing. If not called, it breaks the MRO chain of inheritance and complicates the mixin/class building. This must be a linting rule.
`attrs` cannot use multiple inheritance when both parents introduce their attributes (as documented). Only one side can inherit the attributes, other bases must be pure interfaces/protocols. Reimplement the `ExprNode.type` via properties to exclude it from the sight of `attrs`.
We are going to do strict type checking. The default values of fields that clearly contradict the declared types is an error for MyPy and all other type checkers and IDEs. Remove the implicit behaviour and make nullable fields explicitly declared as such.
`attrs` is much more beneficial: * `attrs` is supported by type checkers, such as MyPy & IDEs * `attrs` is widely used and industry-proven * `attrs` is explicit in its declarations, there is no magic * `attrs` has slots But mainly for the first item — type checking by type checkers. Those that were runtype classes, are frozen. Those that were not, are unfrozen for now, but we can freeze them later if and where it works (the stricter, the better).
Since we now use `attrs` for some classes, let's use `attrs` for them all — at least those belonging to the same hierarchies. This will ensure that all classes are slotted and will strictly check that we define attributes properly, especially in cases of multiple inheritance. Except for Pydantic models and Python exceptions. Despite the attrs classes are not frozen by default, we keep it explicitly stated, so that we see which classes were or were not frozen before the switch from runtype to attrs. We can later freeze more classes if/when it works (the stricter, the better). For this reason, we never unfreeze classes that were previously frozen. ->?
nolar
force-pushed
the
attrs-instead-of-runtype
branch
from
6386699
to
ee37648
Compare
|
@nolar Just for the record, runtype does support forward-refs and slots, you only had to upgrade to a later version. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A replay of accidentally closed #720.
Short-term goal: resolve an issue where
runtypecannot checkisinstance()check, where the type is a string"Database"coming fromCompilar.database: "Database".Technically, it is a
typing.ForwardRef[]and must be resolved to actual classes on usage.attrsandpydanticdo that.There is no way to convert it to a regular class, because there is still a circular class dependency
Database<>Compiler.Long-term goal: Since we are moving in the direction of strict type checking, let's make the first step: replace
runtypewith lots of implicit logic and not compatible with MyPy, withattrs, which require explicitness and work fine with MyPy & IDEs.attrsis much more beneficial:attrsis supported by type checkers, such as MyPy & IDEsattrsis widely used and industry-provenattrsis explicit in its declarations, there is no magicattrshas slotsBut mainly for the first item — type checking by type checkers.
And since we switch to
attrsfor already dataclass-like types, also expand it to all other (unannotated) classes to ensure proper attribute management in multiple inheritance.In particular, in the last commit, there are a few cases where the code was ambiguous and had to be resolved by minor remakes.
It is better to review commit-by-commit.