iiasa / message-ix-models

Tools for the MESSAGEix-GLOBIOM family of models
https://docs.messageix.org/models
Apache License 2.0
17 stars 33 forks source link

Remove Python 3.10-exclusive kw_only dataclass argument temporarily #146

Closed glatterf42 closed 8 months ago

glatterf42 commented 8 months ago

As discovered by @behnam-zakeri, our current code should either require python >= 3.10, or remove the dataclass() argument kw_only until this is the case. Should we leave a note somewhere that we strongly encourage users to still use the syntax you were trying to enforce, @khaeru?

Please note: this PR leaves a TODO comment to come back to it. The other changes are small notes that mypy complained about.

How to review

PR checklist

glatterf42 commented 8 months ago

Edit: the ScenarioInfo.scenario_obj field had the kw_only argument, too, and I left it in at first because I thought it wouldn't hurt to keep it for the future (since it was set to its default False value. But python < 3.10 might still complain about not knowing this argument, so I commented it out as well.

codecov[bot] commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (af092b1) 74.9% compared to head (cbf7b71) 74.9%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #146 +/- ## ===================================== Coverage 74.9% 74.9% ===================================== Files 91 91 Lines 6084 6084 ===================================== Hits 4558 4558 Misses 1526 1526 ``` | [Files](https://app.codecov.io/gh/iiasa/message-ix-models/pull/146?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iiasa) | Coverage Δ | | |---|---|---| | [message\_ix\_models/util/scenarioinfo.py](https://app.codecov.io/gh/iiasa/message-ix-models/pull/146?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iiasa#diff-bWVzc2FnZV9peF9tb2RlbHMvdXRpbC9zY2VuYXJpb2luZm8ucHk=) | `100.0% <100.0%> (ø)` | |
behnam-zakeri commented 8 months ago

Edit: the ScenarioInfo.scenario_obj field had the kw_only argument, too, and I left it in at first because I thought it wouldn't hurt to keep it for the future (since it was set to its default False value. But python < 3.10 might still complain about not knowing this argument, so I commented it out as well.

Thanks @glatterf42. Indeed, that instance needs to be commented/removed too.

glatterf42 commented 8 months ago

Thanks for confirming. I think the codecov failure is due to the two pushes to this branch being in quick succession, this sometimes leads to coverage reports being not properly collected. The apparently uncovered lines are in a file I didn't touch, at least.

glatterf42 commented 8 months ago

Thanks, that would have defeated the purpose of this PR.

I'd leave the expansion of the docs for another PR to avoid scope creep.