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

Support C# 8 nullable reference types #1240

Merged
merged 14 commits into from Feb 2, 2019

Conversation

Projects
None yet
1 participant
@jskeet
Copy link
Member

jskeet commented Dec 17, 2018

This PR is now "complete" in terms of applying nullable reference types to all the projects in NodaTime-All.sln.

Further work:

  • Remove the existing nullability annotations and tests for them (probably, anyway)
  • Review complete API after merging; that will be simpler than looking at the diff
  • Update web-based projects too

It's expected that AppVeyor fails right now, as it doesn't support VS2019 preview 2 just yet. I'm happy to merge based on Travis and local builds. (It's not like this code will be going GA any time soon.)

@jskeet jskeet self-assigned this Dec 17, 2018

@jskeet jskeet force-pushed the jskeet:nullable-references branch 2 times, most recently from de23b06 to 2567eb2 Dec 18, 2018

@jskeet jskeet force-pushed the jskeet:nullable-references branch 2 times, most recently from 8f637d2 to e31b076 Jan 29, 2019

@jskeet jskeet changed the title (Not for submission yet) Nullable references Support C# 8 nullable reference types Jan 30, 2019

jskeet added some commits Dec 17, 2018

First pass at applying nullable annotations within the production code
- No other projects (including Test and Testing) have been annotated yet
- I haven't done a full public API pass to validate the decisions
- We probably want to check all the overrides of (e.g) Equals(object)
Convert CommandLine library to nullable reference types
Additionally, do a bunch of formatting tidy-up etc. This isn't exhaustive.

There's quite a lot here that's not really how I'd like it, but this is effectively legacy code - and never used outside tools, which aren't published. If something goes bang, that's not much of a problem.
Remove all nullable TODOs
- Using ! in ParseResult is okay, due to the (now documented) invariant
- Mads confirmed that implementing IEquatable<Foo?> is appropriate
- Roslyn bug was fixed for preview 2
Enable nullable reference types for NodaTime.Test
*Lots* of changes here, which was expected. Additionally, some changes to NodaTime, indicating that we haven't really done a thorough enough review of the surface yet.
Make cref references in doc comments match in nullability
This allows docfx to generate metadata. It generates it incorrectly, but that's a separate matter...

@jskeet jskeet force-pushed the jskeet:nullable-references branch from d61ed1a to 083fc9e Feb 1, 2019

@jskeet

This comment has been minimized.

Copy link
Member Author

jskeet commented Feb 1, 2019

Nullable reference types currently confuse docfx in terms of generated metadata. I'll see if I can fix this up with tooling. I'll do that as a separate PR, but it'll basically block this for now.

@jskeet

This comment has been minimized.

Copy link
Member Author

jskeet commented Feb 2, 2019

Workaround tooling is in #1272, so we can now merge this (then that).

@jskeet jskeet merged commit 6312168 into nodatime:master Feb 2, 2019

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jskeet jskeet deleted the jskeet:nullable-references branch Feb 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment