openscm / scmdata

Handling of Simple Climate Model data
https://scmdata.readthedocs.io
BSD 3-Clause "New" or "Revised" License
8 stars 5 forks source link

refactor: Always return results from `inplace` operations. #232

Closed lewisjared closed 1 year ago

lewisjared commented 1 year ago

Updates operations that could be performed inplace to always return an ScmRun. The inplace argument dictates whether it is a copy or not.

This also fixes a typing issue for run_append which had a return type of BaseScmRun | None. It now returns the type of the items that are being appended.

The current implementation isn't perfect, as I don't believe you differentiate the type for the first item vs the rest of the list. Python assumes that a list has a homogenous type which results in a mixed type list being typed as list[A | B].

For example:

a: ScmRun = ...
b: BaseScmRun = ...

res = run_append([a, b])

res will have an inferred type of ScmRun | BaseScmRun rather than the expected ScmRun. Not perfect, but if we type it as a tuple we can differentiate the first type vs the rest. @mikapfl you might have a better solution for this.

Typing as a tuple wouldn't break any functionality, but would result in type errors in downstream code.

Closes #230

Pull request

Please confirm that this pull request has done the following:

Adding to CHANGELOG.rst

Please add a single line in the changelog notes similar to one of the following:

- (`#XX <https://github.com/openscm/scmdata/pull/XX>`_) Added feature which does something
- (`#XX <https://github.com/openscm/scmdata/pull/XX>`_) Fixed bug identified in (`#YY <https://github.com/openscm/scmdata/issues/YY>`_)
lewisjared commented 1 year ago

I'm going to follow this MR with a rework of the typing for all public functions. A lot of functions also return Self and aren't currently typed as such

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.03 :tada:

Comparison is base (753e83f) 95.77% compared to head (b7f45bb) 95.81%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #232 +/- ## ========================================== + Coverage 95.77% 95.81% +0.03% ========================================== Files 23 23 Lines 2108 2104 -4 Branches 395 390 -5 ========================================== - Hits 2019 2016 -3 Misses 71 71 + Partials 18 17 -1 ``` | [Impacted Files](https://app.codecov.io/gh/openscm/scmdata/pull/232?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openscm) | Coverage Δ | | |---|---|---| | [src/scmdata/run.py](https://app.codecov.io/gh/openscm/scmdata/pull/232?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openscm#diff-c3JjL3NjbWRhdGEvcnVuLnB5) | `96.13% <100.00%> (+0.12%)` | :arrow_up: |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

review-notebook-app[bot] commented 1 year ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

znicholls commented 1 year ago

res will have an inferred type of ScmRun | BaseScmRun rather than the expected ScmRun. Not perfect, but if we type it as a tuple we can differentiate the first type vs the rest. @mikapfl you might have a better solution for this.

Hmm tricky. I could live with the tuple requirement (I assume that just using Iterable or Sequence or something doesn't solve our problem?), but also interested to hear what @mikapfl thinks as my typing experience is limited.

mikapfl commented 1 year ago

I think the Iterable[T] is fine. It is correct that it leads to wrong mixed inferred types for the run_append([ScmRun, BaseScmRun]) call, but, honestly, that is borderline incorrect usage anyway, I think. What is the use case for concatenating mixed types here?