-
Notifications
You must be signed in to change notification settings - Fork 15.5k
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
Set all but main project to nullable #10170
Conversation
Thanks - I'll look at this next week. We'll get there :) I'm not 100% sure that starting at the periphery is going to be the most efficient approach though - because when we change the core later, all the test code will change too. Perhaps we should start with the inner-most code? (While the test project is null-oblivious, that won't affect anything. I'd suggest even for Google.Protobuf we could leave the project itself with nullability disabled, and enable it on a class-by-class basis until we've got a critical mass.) |
Started this way simply to reduce later diffs: There are a lot of test code changes that did not need to be ported over from the other PR, but will be introduced incrementally once this goes in. This cuts the initial noise to make those PRs more focused on jus the related API changes. |
(By which I mean after this PR, can start converting one or a few classes per PR and the diffs will just be related to those classes) |
Okay, I see what you mean. Yup, that's fine. (There are multiple ways of approaching this kind of change.) It's on my to-do list for next week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few nits - but generally looking good. Sorry this took so long to get round to!
csharp/src/Google.Protobuf.Benchmarks/ParseRawPrimitivesBenchmark.cs
Outdated
Show resolved
Hide resolved
csharp/src/Google.Protobuf.Benchmarks/WriteRawPrimitivesBenchmark.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running Kokoro tests now...
496de86
to
4cf6014
Compare
This is the wrong way around to do this. The main project should be made nullable first, then other projects. Otherwise, you'll need to react to all the nullable changes as main project is annotated. |
@JamesNK: I don't think there's an objective "right" or "wrong" here - just different pros and cons. I completely acknowledge that this way round means multiple iterations of the test projects - but I see that as a benefit. By doing it that way, you see the changes required in tests at the same time as the changes required in the main project, without any distractions from non-protobuf changes required for the tests. That, to me, is a good way of getting a "sniff test" about whether the changes to the main project are appropriate - if the required changes to the tests seem reasonable, that's encouraging. It's a lot harder to pick those out when all the changes required to the tests come in the same commit. |
4cf6014
to
1afa4ad
Compare
1afa4ad
to
bea7f72
Compare
We triage inactive PRs and issues in order to make it easier to find active work. If this PR should remain active, please add a comment. This PR is labeled |
We triage inactive PRs and issues in order to make it easier to find active work. If this PR should remain active or becomes active again, please reopen it. This PR was closed and archived because there has been no new activity in the 14 days since the |
Start with the periphery including test code.