Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #356 +/- ##
===========================================
+ Coverage 71.14% 71.24% +0.09%
===========================================
Files 44 44
Lines 5844 5870 +26
Branches 1158 1158
===========================================
+ Hits 4158 4182 +24
- Misses 1364 1366 +2
Partials 322 322 ☔ View full report in Codecov by Sentry. |
alexdewar
left a comment
alexdewar
left a comment
There was a problem hiding this comment.
I've skimmed this and it mostly looks totally fine (and the newer syntax is much nicer to look at!).
I am a little concerned that there are some places where you're writing union types as type_a | type_b, which is only available in Python 3.10. Oddly, none of our workflows have caught this (maybe because we don't have explicit tests for the type hints?), but I'm a bit concerned things will break at runtime for Python 3.9 users.
| "outputs": [], | ||
| "source": [ | ||
| "from typing import List, Optional, Text\n", | ||
| "from typing import Optional\n", |
There was a problem hiding this comment.
I guess we have to wait for a newer version of Python until we can write Optionals as my_type | None!
| market: xr.Dataset, | ||
| technologies: xr.Dataset, | ||
| year: Optional[int] = None, | ||
| year: int | None = None, |
There was a problem hiding this comment.
I think writing union types this way is a Python 3.10 thing, alas: https://docs.python.org/3/library/typing.html#typing.Union
There was a problem hiding this comment.
It's a bit weird that the Python 3.9 runners don't seem to be complaining... Maybe these bits aren't covered by tests?
There was a problem hiding this comment.
These sort of things are checked by mypy which we are not running in MUSE because it largely lacks of consistent typing information. Maybe at runtime these things do not matter at all?
What annoys me is that I set as target python version 3.9, so if this is a 3.10 feature, it should not have been changed :(
There was a problem hiding this comment.
Ugh, that is annoying. I guess it's a ruff bug? 😞
Strangely, there are plenty of other places where it's left Optional[int]s alone.
I suppose one option would be to do this first upgrade with pyupgrade itself, which presumably doesn't have this problem? I guess the ruff hook might then change these bits again, though. Perhaps if it's being really stubborn we should just use the pyupgrade pre-commit hook.
There was a problem hiding this comment.
I've just found the trick in here. Esentially, you can use | with no problem from python 3.7 as long as you do from __future__ import annotations which we do in all the files that ruff changed.
In summary, these changes are safe to go.
There was a problem hiding this comment.
In the file where that was not changed it was because we did not have that import statement.
There was a problem hiding this comment.
Ohhh I see. Well that's a relief
3 participants
Description
pyupgradeis now enabled inruff, using python 3.9 as the target since we want to keep the compatibility with it. As usual with these things, a lot of files are changed, but otherwise you should only worried aboutpyproject.toml.Fixes #293
Type of change
Please add a line in the relevant section of
CHANGELOG.md to
document the change (include PR #) - note reverse order of PR #s.
Key checklist
$ python -m pytest$ python -m sphinx -b html docs docs/buildFurther checks