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

Use C# 8 nullable reference types #60

Closed
Happypig375 opened this issue Apr 25, 2020 · 12 comments · Fixed by #147
Closed

Use C# 8 nullable reference types #60

Happypig375 opened this issue Apr 25, 2020 · 12 comments · Fixed by #147
Labels
Proposal Contribute to the project by proposing some improvements

Comments

@Happypig375
Copy link
Member

Reference https://github.com/manuelroemer/Nullable.

@WhiteBlackGoose
Copy link
Member

What's the point of using it?

@Happypig375
Copy link
Member Author

Making clear whether parameters are nullable lets downstream users avoid unintentionally passing nulls into methods.

@Happypig375
Copy link
Member Author

This is not done. Request reopen.

@Happypig375
Copy link
Member Author

𝖳𝗁𝗂𝗌 𝗂𝗌 𝗇𝗈𝗍 𝖽𝗈𝗇𝖾. 𝖱𝖾𝗊𝗎𝖾𝗌𝗍 𝗋𝖾𝗈𝗉𝖾𝗇.

@MomoDeve
Copy link
Member

MomoDeve commented Jun 19, 2020

Could you at least put examples where this feature can be used in our library? From my experience, almost always methods parameters must not be null in AM

@Happypig375
Copy link
Member Author

Happypig375 commented Jun 19, 2020

From my experience, almost always methods parameters must not be null in AM

Then that is great! After enabling nullable reference types, little code changes will be required.

@Happypig375
Copy link
Member Author

Happypig375 commented Jun 19, 2020

Could you at least put examples where this feature can be used in our library?

Put

<LangVersion>8.0</LangVersion>
<Nullable>enable</Nullable>
<WarningsAsErrors>nullable</WarningsAsErrors>

in the csprojs.

@siriak
Copy link

siriak commented Jun 19, 2020

@Happypig375 maybe you could do that and make a PR?

@Happypig375
Copy link
Member Author

@siriak The green light is needed or LayoutFarm/Typography#192 will happen again.

@siriak
Copy link

siriak commented Jun 21, 2020

@Happypig375 agree, good point.

@MomoDeve
Copy link
Member

MomoDeve commented Jun 21, 2020

We will look at the changes. If all unit tests are passed, we will merge it.
You can split your pull request into multiple ones, to let us review changes in portions

@WhiteBlackGoose WhiteBlackGoose added the Proposal Contribute to the project by proposing some improvements label Jun 24, 2020
@Happypig375
Copy link
Member Author

I have started to implement this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Proposal Contribute to the project by proposing some improvements
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants