Skip to content

**BREAKING** Type Stability for MOInputs #405

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from

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