Address Feedback from implementation #22359
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files🚀 New features to boost your workflow:
|
AAraKKe
left a comment
AAraKKe
left a comment
There was a problem hiding this comment.
Thanks @steveny91 !! Sorry I couldn't take a look yesterday. Most of the comments are suggestions except for the request on the app mock.
My Feedback Legend
Here's a quick guide to the prefixes I use in my comments:
praise: no action needed, just celebrate!
note: just a comment/information, no need to take any action.
question: I need clarification or I'm seeking to understand your approach.
nit: A minor, non-blocking issue (e.g., style, typo). Feel free to ignore.
suggestion: I'm proposing an improvement. This is optional but recommended.
request: A change I believe is necessary before this can be merged.
The only blocking comments are request, any other type of comment can be applied at discretion of the developer.
ddev/src/ddev/cli/upgrade_check.py
Outdated
|
|
||
|
|
||
| def read_last_run(cache_file=CACHE_FILE): | ||
| def read_last_run(cache_file: Path) -> Tuple[Optional[Version], Optional[datetime]]: |
There was a problem hiding this comment.
suggestion: while valid this is not the current way of typing this and will eventually be deprecated. Types of standard collections, like tuple or dict can now be typed without any typing import (PEP 585). The same for Optional which now can be typed with union types (PEP 604.
Also, are both optional? I.e. can we have one being None while the other is not? If the answer is that either both are None or both have values, I would probably type it saying that the return value is tuple(Version, datetime) | None and then check for None. That way, if it is not None we are sure that the value is valid. Otherwise having one element being not None does not tell us anything about the other.
For the current setup, this would be the typing.
| def read_last_run(cache_file: Path) -> Tuple[Optional[Version], Optional[datetime]]: | |
| def read_last_run(cache_file: Path) -> tuple(Version | None, datetime | None]: |
There was a problem hiding this comment.
either both are None or both have values
This is the right scenario. Either you load both from the cache or you load None from the cache. I refactored it a bit to make it more clear.
ddev/src/ddev/cli/upgrade_check.py
Outdated
|
|
||
|
|
||
| def write_last_run(version, date, cache_file=CACHE_FILE): | ||
| def write_last_run(version: Version, date: datetime, cache_file: Path) -> None: |
There was a problem hiding this comment.
suggestion: while -> None is valid, it is normally better just not to add the typing information when we return None. It is inferred and is less to write.
ddev/src/ddev/cli/upgrade_check.py
Outdated
|
|
||
| def exit_handler(app, msg): | ||
| return app.display_info(msg, highlight=False) | ||
| def exit_handler(app, latest_version: Version, current_version: Version) -> None: |
There was a problem hiding this comment.
note: left the app without typing. You can import Application just for type annotations with the following
from typing import TYPE_CHECKING
if TYPE_CHECKING:
from ddev.cli.application import ApplicationThis avoids loading the Application on runtime but allows to get all the benefits from typing. Same for all the references to app.
ddev/tests/cli/test_upgrade_check.py
Outdated
| assert default_cache_file().as_posix().endswith("/tmp/ddev-cache/upgrade_check.json") | ||
|
|
||
| def test_upgrade_uses_default_cache_file_when_cache_file_is_none(self, mocker): | ||
| app = MagicMock() |
There was a problem hiding this comment.
request: we have an app fixture for ddev. You can just inject it on the test. Just because if we modify the test and start doing wrong things with the app object we won't know because the mock will swallow everything.
There was a problem hiding this comment.
I tried this. Let me know what you think.
ddev/tests/cli/test_upgrade_check.py
Outdated
| def test_upgrade_skips_for_dev_versions(self, tmp_path, mocker): | ||
| cache_file = tmp_path / "upgrade_check.json" | ||
| app = MagicMock() | ||
| mock_get = mocker.patch("ddev.cli.upgrade_check.requests.get") |
There was a problem hiding this comment.
suggestion: I see that we use these mocks a lot in all tests almost, why not creating a fixture with them for this module?
@pytest.fixture
def mock_get(mocker):
return mocker.patch("ddev.cli.upgrade_check.requests.get")Also, didn't say anything but it would be nice to have the tests as standard pytest instead of within a class as well to be consistent with the rest of the new stuff we are doing. Classes are supported by pytest to support the runner with unittests but it is normally better to have the different set of tests grouped by modules instead of classes. It can lead to huge test files with multiple classes within them.
What does this PR do?