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

Microsoft.Identity.Web - Implement C# 8.0 nullable standard #249

Closed
ghost opened this issue Jan 8, 2020 · 5 comments
Closed

Microsoft.Identity.Web - Implement C# 8.0 nullable standard #249

ghost opened this issue Jan 8, 2020 · 5 comments

Comments

@ghost
Copy link

ghost commented Jan 8, 2020

Even if the project is not C# 8.0, consider implementing the nullable standard by suffixing ? to all parameters and returns that may be NULL.

This will be a breaking changing internal to the library & to consuming code. Therefore, consider publishing the changes only during a major cycle.

@jmprieur
Copy link
Contributor

jmprieur commented Jan 8, 2020

@sujayvsarma can you be more specific about which parameters you'd want to be null?

@ghost
Copy link
Author

ghost commented Jan 8, 2020

Not that I would want to make null, but it creates a distinction between what the code would consider to maybe null or will never be null.

For example, in Microsoft.Identity.Web > TokenAcquisition.cs, line 165, you are checking if the parameter scopes is null. If the GetAccessTokenOnBehalfOfUserAsync expects to receive scopes as null sometime, it would define its signature as:

public async Task<string> GetAccessTokenOnBehalfOfUserAsync(
            IEnumerable<string>? scopes,
            string tenant = null)

Note the ? at the end of the IEnumerable<string>. That says to the calling developer that scopes could be passed in as NULL. Otherwise it tells them that scopes cannot be NULL there.

Simply add this to your Microsoft.Identity.Web.csproj and watch Visual Studio make those suggestions for you:

<LangVersion>8.0</LangVersion>
<Nullable>enable</Nullable>

Fixing those suggestions is a breaking change.

@ghost
Copy link
Author

ghost commented Jan 9, 2020

Fyi, I have managed to implement these suggestions and managed to remove & clean up some code as well. If you're interested, I'll hook up a PR or send you the zip somewhere ?

@jmprieur
Copy link
Contributor

jmprieur commented Jan 9, 2020

Sure, @sujayvsarma, feel free to propose a PR.

@jennyf19
Copy link
Contributor

issue tracked in microsoft identity web repo: AzureAD/microsoft-identity-web#15

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

No branches or pull requests

2 participants