Skip to content
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

Nullability fixes for chaos outcome strategy #1982

Merged
merged 1 commit into from
May 10, 2024

Conversation

martintmk
Copy link
Contributor

@martintmk martintmk commented Feb 21, 2024

Details on the issue fix or feature implementation

Fixing some incorrect nullability annotatations.

Confirm the following

  • I started this PR by branching from the head of the default branch
  • I have targeted the PR to merge into the default branch
  • I have included unit tests for the issue/feature
  • I have successfully run a local build

@peter-csala
Copy link
Contributor

peter-csala commented Mar 6, 2024

@martintmk If you want I can help with this PR by fixing the tests (and resolving the merge conflict).

@martintmk
Copy link
Contributor Author

@martintmk If you want I can help with this PR by fixing the tests (and resolving the merge conflict).

That would be great as I won't be able to touch this in next two weeks (vacation).

@peter-csala
Copy link
Contributor

@martintmk If you want I can help with this PR by fixing the tests (and resolving the merge conflict).

That would be great as I won't be able to touch this in next two weeks (vacation).

Sure, no problem. Enjoy your vacation! 😏

Please allow me (@peter-csala) to contribute to your branch.

@martincostello
Copy link
Member

The branch is here rather than in a fork, so I'm not sure that's doable.

If you want to pick up the work, copy the branch locally into your fork and start a new PR.

@peter-csala
Copy link
Contributor

The branch is here rather than in a fork, so I'm not sure that's doable.

If you want to pick up the work, copy the branch locally into your fork and start a new PR.

Ohh okay, I thought Martin was working on a fork as well.

I'll create a new PR then.

@martintmk martintmk force-pushed the mtomka/nullability-fixes branch from 8de46fa to 61cfa3a Compare May 10, 2024 11:47
@martintmk martintmk changed the title [WIP] Nullability fixes for chaos outcome strategy Nullability fixes for chaos outcome strategy May 10, 2024
@martintmk martintmk marked this pull request as ready for review May 10, 2024 11:49
@martintmk
Copy link
Contributor Author

@martincostello This one should be ready, after this is merged we can go ahead with 8.4.0

@martincostello martincostello added this to the v8.4.0 milestone May 10, 2024
Copy link
Member

@martincostello martincostello left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced this was worth doing in the end as the net effect is just a little less code at the expense of allowing the compiler to assign null when that isn't actually allowed, so what is there already feels more correct to me to be honest, but if it makes it consistent with all the other options types, we might as well change it.

@martincostello martincostello merged commit 11f84b2 into main May 10, 2024
16 checks passed
@martincostello martincostello deleted the mtomka/nullability-fixes branch May 10, 2024 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants