Finish typing of aiidalab.app and aiidalab.__main__ modules#453
Finish typing of aiidalab.app and aiidalab.__main__ modules#453danielhollas merged 8 commits intoaiidalab:mainfrom
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #453 +/- ##
==========================================
- Coverage 72.00% 71.76% -0.25%
==========================================
Files 18 18
Lines 1611 1622 +11
==========================================
+ Hits 1160 1164 +4
- Misses 451 458 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| self.app = app | ||
|
|
||
| def on_any_event(self, event): | ||
| def on_any_event(self, event: FileSystemEvent) -> None: |
There was a problem hiding this comment.
I looked inside the watchdog package to see the correct type here.
| - types-requests | ||
| - types-toml | ||
| - types-click | ||
| - traitlets~=5.0 |
There was a problem hiding this comment.
pre-commit runs mypy from an isolated virtual environment so it doesn't have access to the installed dependencies, and thus we need to add them here manually. This is highly problematic, as we are essentially duplicating info from setup.cfg. In the future, we probably have to switch to an approach used in aiida-core, where mypy is installed as dev dependency of aiidalab, instead of being installed by pre-commit.
| self.dependencies_to_install = self._app.find_dependencies_to_install( | ||
| self.version_to_install | ||
| ) | ||
| if self.version_to_install: |
There was a problem hiding this comment.
This could be None so mypy got rightfully mad that we're passing a possible None value to a function that did not expect it. In other words, this was a bug.
|
I released watchdog v5.0.0 that contains a full types coverage, it should ease your job here :) And, again, thank you for testing the |
|
@superstar54 will you have time to look at this? I'd like to include this in the next release this week. |
|
|
||
| from . import __version__ | ||
| from .app import AppVersion | ||
| from .app import _AiidaLabApp as AiidaLabApp |
There was a problem hiding this comment.
This renaming was super confusing.
unkcpz
left a comment
There was a problem hiding this comment.
Thanks @danielhollas, just a minor request, all good then.
|
|
||
| @contextmanager | ||
| def _mock_schemas_endpoints(): | ||
| def _mock_schemas_endpoints() -> Generator[None, None, None]: |
There was a problem hiding this comment.
| def _mock_schemas_endpoints() -> Generator[None, None, None]: | |
| def _mock_schemas_endpoints() -> Iterator[None]: |
I think this is simpler?
There was a problem hiding this comment.
Hmm, I guess you're right, and mypy accepts this. However, since we use the Generator type in many other places now, I'd like to keep things consistent for now. Later if we like we can go through and simplify this accross the code base.
|
|
||
| @contextmanager | ||
| def _show_busy(self): | ||
| def _show_busy(self) -> Generator[None, None, None]: |
There was a problem hiding this comment.
| def _show_busy(self) -> Generator[None, None, None]: | |
| def _show_busy(self) -> Iterator[None]: |
Towards #363, almost done!