Ignore upgrade check in dev versions and also fix windows cache location #22342
Conversation
steveny91
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. 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 !! Added some comments and a general set of requests not related with this comment but with the original implementation.
| return Path(user_cache_dir('ddev', appauthor=False)).expanduser() / "upgrade_check.json" | ||
|
|
||
|
|
||
| CACHE_FILE = default_cache_file() |
There was a problem hiding this comment.
suggestion: I would probably remove this constant and made the cache file parameter None by default. The reason is that even though we are importing platformdirs in the method itself, this method will be called everytime we import the module, defeating the purpose of importing platformdirs only when needed.
This is a suggestion because we already import platformdirs to work with the config file, which is needed on startup of the cli, so we are really not benefiting a lot. But to be consistent with minimizing the number of imports we do I think we should move in that direction.
| try: | ||
| version_to_cache = last_version if last_version else current_version | ||
| write_last_run(version_to_cache, date_now, cache_file) | ||
| except OSError: |
There was a problem hiding this comment.
suggestion: when we do try/except -> pass there is a more idiomatic way of doing it using the contextlib.supress method.
There was a problem hiding this comment.
request: these are general requests on the overall file. We should build this to follow our cli design. This means that:
- Remove logging import, we do not log with logging and using
debug,infoor other logging levels cannot be controlled inddev. We use our console printing for this controlled by verbosity levels to be able to format the output. - We should inject the console, or our
Terminalwrapper, which is whatApplicationinherits from, into this command and use the proper methods to write to terminal. I.e. inject the app itself into theupgrade_checkmethod and use its owndisplay_*methods to print information on screen. - This remove the need of using
ANSIescape codes when printing with formatting. - Annotate all methods.
What does this PR do?