Skip to content

MOInput Type Stability #465

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

Merged
merged 10 commits into from
Aug 14, 2022
Merged

Conversation

willtebbutt
Copy link
Member

Summary
Makes the type of out_dim field of MOInput a type parameter of MOInput. This has been done in a non-breaking way.

Proposed changes

See summary

  • ...

What alternatives have you considered?

#405 consides an alternative approach in which the type is constrained to Int. This has the disadvantage of being more constrained, and constituting a break change. The potential advantage over the approach used here is slight changes in compile time, but my instinct is that these will be negligible.

Breaking changes

None

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@theogf
Copy link
Member

theogf commented Aug 14, 2022

Maybe add a @inferred test?

@codecov
Copy link

codecov bot commented Aug 14, 2022

Codecov Report

Merging #465 (f1f181b) into master (bda66f8) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #465   +/-   ##
=======================================
  Coverage   93.16%   93.16%           
=======================================
  Files          52       52           
  Lines        1259     1259           
=======================================
  Hits         1173     1173           
  Misses         86       86           
Impacted Files Coverage Δ
src/mokernels/moinput.jl 100.00% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Will Tebbutt and others added 3 commits August 14, 2022 14:50
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@willtebbutt
Copy link
Member Author

@theogf done!

@willtebbutt
Copy link
Member Author

Thanks for the swift review @theogf -- will merge when CI passes

@willtebbutt
Copy link
Member Author

@theogf looks like Others on 1.3 is failing on the doctests -- the formatting of the type printing appears to be different for that version. I don't know how tests were passing previously, because presumably this hasn't changed... 😬

Any thoughts?

@theogf
Copy link
Member

theogf commented Aug 14, 2022

Probably the doctestfilter don't catch so many types or something? Didn't we mention dropping 1.3 anyways?

@willtebbutt
Copy link
Member Author

Cool. I'm just going to drop CI for 1.3

@willtebbutt
Copy link
Member Author

willtebbutt commented Aug 14, 2022

@theogf any idea how I tell github to not require the 1.3 tests to be run?

Nvm, figured it out :)

@willtebbutt willtebbutt merged commit b5af459 into master Aug 14, 2022
@willtebbutt willtebbutt deleted the wct/moinput-integer-type-stability branch August 14, 2022 15:35
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.

2 participants