Skip to content

**BREAKING** Type Stability for MOInputs#405

Closed
willtebbutt wants to merge 4 commits intomasterfrom
wct/moinput-type-stability
Closed

**BREAKING** Type Stability for MOInputs#405
willtebbutt wants to merge 4 commits intomasterfrom
wct/moinput-type-stability

Conversation

@willtebbutt
Copy link
Member

Summary

Make input collection types for isotopic multi-output things type stable.

Proposed changes

Change the Integer type constraint in MOInputIsotopicByFeatures and MOInputIsotopicByOutputs withInts.

What alternatives have you considered?

Could have parametrised the type to allow any Integer, but I think this is probably fine.

Breaking changes

If anyone is currently using anything other than an Int to specify the number of outputs, they'll need to change it to an Int. I can't imagine that this happens very often, so effects should be minimal.

@codecov
Copy link

codecov bot commented Nov 10, 2021

Codecov Report

Merging #405 (364f957) into master (992b665) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #405   +/-   ##
=======================================
  Coverage   92.74%   92.74%           
=======================================
  Files          52       52           
  Lines        1199     1199           
=======================================
  Hits         1112     1112           
  Misses         87       87           
Impacted Files Coverage Δ
src/mokernels/moinput.jl 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 992b665...364f957. Read the comment docs.

@st--
Copy link
Member

st-- commented Nov 19, 2021

Seems fine to me.
For the doc issue, you need to update the Manifests of doc/ and examples/*/.
For the test issue, see #409

@willtebbutt
Copy link
Member Author

willtebbutt commented Jan 10, 2022

@theogf @st-- there are a number of other things that we've wanted to address in the next breaking release for a while: #338

I don't have time to push these through right now, so I'm going to hold off on merging this until we've got them in place, either in this PR or another, so that we don't have to block patch releases on master.

@theogf
Copy link
Member

theogf commented Jan 10, 2022

Should we generally have a branch breaking_master where we can merge these breaking PRs?

@willtebbutt
Copy link
Member Author

Closed in favour of #465 , which is non-breaking

@willtebbutt willtebbutt deleted the wct/moinput-type-stability branch August 14, 2022 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants