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

Enabled C# 8 #527

Merged
merged 1 commit into from
Mar 5, 2020
Merged

Enabled C# 8 #527

merged 1 commit into from
Mar 5, 2020

Conversation

lukasz-pyrzyk
Copy link
Contributor

Fixes #525

@SergeyKanzhelev
Copy link
Member

Is there bunch of warnings around nullable? I remember @bruno-garcia said there will be significant work effort to switch to 8.0

@SergeyKanzhelev SergeyKanzhelev merged commit d48aad6 into open-telemetry:master Mar 5, 2020
@lukasz-pyrzyk
Copy link
Contributor Author

I don't have any warnings about nullable. As far as I know, we need to enable them globally / for project / for scope to get those warnings

@lukasz-pyrzyk lukasz-pyrzyk deleted the feature/csharp8 branch March 5, 2020 17:58
@bruno-garcia
Copy link
Member

Happy we got C# 8 enabled already. Nullables are opt-in per project through: <Nullable>enable</Nullable>. I didn't start with nullable ref types since it'll be a considerable amount of work and I don't want to do 1 project and disappear.

@lukasz-pyrzyk if you're are willing to do that, I was planning to do project by project but it's up for grabs. Could go file by file although not my favorite approach, i.e: #nullable enable

@lukasz-pyrzyk
Copy link
Contributor Author

Thanks. I think I'm not gonna pick up nullable reference types right now, I don't have enough knowledge about the project and time to do it correctly

@bruno-garcia
Copy link
Member

The hardest part is when you get to a point where it's not simply adding ? or some attribute.
You actually need to change the code by making it non nullable and refactor all usage of that. Or when something is not supposed to be null but doesn't get assigned via ctor (i.e: EntityFramework).

Also it's worth taking a look at those attribute likes NotNullWhen(.. for cases like TrySomething and out parameters.

For reference: #380

While doing this I found a bug on GetHashCode in Trace/Status.cs. Since Description can be null, it would throw.

@bruno-garcia
Copy link
Member

Oh, just read the I'm not gonna. Sorry.
Yeah it's in my TODO list for 3 months now :(

@lukasz-pyrzyk
Copy link
Contributor Author

The hardest part is when you get to a point where it's not simply adding ? or some attribute.
You actually need to change the code by making it non nullable and refactor all usage of that. Or when something is not supposed to be null but doesn't get assigned via ctor (i.e: EntityFramework).

It's true. Doing it right is difficult.

Oh, just read the I'm not gonna. Sorry.
Yeah it's in my TODO list for 3 months now :(

Maybe, someday! 😄

Yun-Ting added a commit to Yun-Ting/opentelemetry-dotnet that referenced this pull request Oct 13, 2022
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.

Migration to C# 8
3 participants