-
Notifications
You must be signed in to change notification settings - Fork 2
feat: enabled nullability and changed formatting standards to our editorconfig #68
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
base: master
Are you sure you want to change the base?
Conversation
remove conditional blocks in csproj
| # CA2007: Consider calling ConfigureAwait on the awaited task | ||
| # In almost all cases in dotnet core this is unnecessary as there is syncronization context | ||
| dotnet_diagnostic.CA2007.severity = none No newline at end of file | ||
| # In almost all cases in dotnet core this is unnecessary as there is no synchronization context |
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.
HA!
|
|
||
|
|
||
| private Result<int> ConstructInt() | ||
| private static Result<int> ConstructInt() |
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.
Did you mean to leave no empty lines here?
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.
I did not
| /// The un-nested result of inspecting the outer result for error and returning it if it exists or the nested result itself otherwise | ||
| /// </returns> | ||
| public static Result<TValue, TErr> Flatten<TValue, TErr>(this Result<Result<TValue, TErr>, TErr> result) | ||
| where TValue : notnull |
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.
Yea I think this is breaking
I was going to say I think code using NRT (like this) would interpret code not using NRT as though all its types were nullable and then this constraint wouldn't be satisfied. Thinking a bit more though I'm not sure what notnull means in non-NRT contexts. If it means value types then this is breaking, but if it means not declared with ? then it's probably fine?
We should just try it. I'll do it after supper if you don't beat me to it.
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.
All good.
The notnull constraint is available starting in C# 8.0 for code compiled in a nullable enable context. Unlike other constraints, if a type argument violates the notnull constraint, the compiler generates a warning instead of an error. Warnings are only generated in a nullable enable context.
- https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/where-generic-type-constraint
| { | ||
| Min = Require(min, nameof(min)); | ||
| Max = Require(max, nameof(max)); | ||
| Min = Range<T>.Require(min, nameof(min)); |
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.
Is qualification required here?
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.
I'll check. This might have just been compiler fighting
| var wType = typeof(W); | ||
|
|
||
| if (!IsAssignableFromMemberType(wType)) | ||
| if (!Variant<T, U>.IsAssignableFromMemberType(wType)) |
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.
Is qualification required here?
Also added nullable type constraints everywhere they were required
PR Question: Should this be a major version increment? It shouldn't break anything but the notnull constraints themselves might cause some issues.
Also updated twae settings - closes #67