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

Set all but main project to nullable #10170

Closed
wants to merge 1 commit into from

Conversation

erikmav
Copy link
Contributor

@erikmav erikmav commented Jun 24, 2022

Start with the periphery including test code.

@erikmav
Copy link
Contributor Author

erikmav commented Jun 24, 2022

@jskeet from #9801

@jskeet
Copy link
Contributor

jskeet commented Jun 24, 2022

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.)

@erikmav
Copy link
Contributor Author

erikmav commented Jun 24, 2022

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.

@erikmav
Copy link
Contributor Author

erikmav commented Jun 24, 2022

(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)

@jskeet
Copy link
Contributor

jskeet commented Jun 24, 2022

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.

Copy link
Contributor

@jskeet jskeet left a 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!

Copy link
Contributor

@jskeet jskeet left a 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...

@JamesNK
Copy link
Contributor

JamesNK commented Oct 28, 2022

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.

@jskeet
Copy link
Contributor

jskeet commented Oct 28, 2022

@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.

Copy link

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 inactive because the last activity was over 90 days ago. This PR will be closed and archived after 14 additional days without activity.

@github-actions github-actions bot added the inactive Denotes the issue/PR has not seen activity in the last 90 days. label Jan 14, 2024
Copy link

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 inactive label was added.

@github-actions github-actions bot closed this Jan 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c# inactive Denotes the issue/PR has not seen activity in the last 90 days.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants