chore(dao): Add generic type for better type checking #24465
Conversation
| @@ -38,6 +38,8 @@ def __init__(self, model_id: int): | |||
|
|
|||
| def run(self) -> Model: | |||
| self.validate() | |||
| assert self._model | |||
There was a problem hiding this comment.
This was needed because now Mypy correctly identified that on line #44 self._model could have been None which didn't adhere to the base class definition.
There was a problem hiding this comment.
I have a draft SIP recommending a slight refactor of the commands which address this problem.
There was a problem hiding this comment.
@betodealmeida I’ve been thinking about the DAOs, commands and validation a bit recently and wonder whether we should adopt the “ask for forgiveness” try approach more often which relies less on Python validation and more on the database schema (foreign keys, uniqueness constraints, etc.) for validation, i.e., the DAO or command would fail if invalid.
The benefits are that we would reduce the Python code footprint and prevent possible race conditions.
| """ | ||
| Generic for saving models | ||
| :raises: DAOCreateFailedError | ||
| """ | ||
| if cls.model_cls is None: | ||
| raise DAOConfigError() | ||
| if not isinstance(instance_model, cls.model_cls): |
There was a problem hiding this comment.
This is no longer needed. The Mypy checks ensure this can't happen.
| try: | ||
| db.session.add(instance_model) | ||
| if commit: | ||
| db.session.commit() | ||
| except SQLAlchemyError as ex: # pragma: no cover | ||
| db.session.rollback() | ||
| raise DAOCreateFailedError(exception=ex) from ex | ||
| return instance_model |
There was a problem hiding this comment.
Inconsistent return type detected with the ChartDAO implementation. Note there's no need to return instance_model as it's passed in as in input.
john-bodley
changed the title
Codecov Report
@@ Coverage Diff @@
## master #24465 +/- ##
==========================================
+ Coverage 68.85% 69.02% +0.17%
==========================================
Files 1901 1901
Lines 73969 74000 +31
Branches 8119 8116 -3
==========================================
+ Hits 50931 51082 +151
+ Misses 20927 20807 -120
Partials 2111 2111
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 4 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
john-bodley
force-pushed
the
john-bodley--dao-typing
branch
from
4b1a2f0
to
7a37ac4
Compare
| def __init_subclass__(cls) -> None: # pylint: disable=arguments-differ | ||
| cls.model_cls = get_args( | ||
| cls.__orig_bases__[0] # type: ignore # pylint: disable=no-member | ||
| )[0] | ||
|
|
There was a problem hiding this comment.
Wow I didn't know Python has added core support for generic types! Adding a reference here for other reviewers: https://peps.python.org/pep-0560/
mistercrunch
added
🏷️ bot
SUMMARY
Per SIP-92 Proposal for restructuring the Python code base #24331 organized all the DAOs to be housed within a shared folder—the result of which highlighted numerous inconsistencies, repetition, and inefficiencies.
This PR (one of many smaller bitesized PRs) improves the typing for DAOs by introducing a generic type to ensure that the model being created, updated, deleted, etc. is in accordance with the DAO.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION