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

Enable nullable? #191

Open
Happypig375 opened this issue Mar 22, 2020 · 13 comments · May be fixed by #192
Open

Enable nullable? #191

Happypig375 opened this issue Mar 22, 2020 · 13 comments · May be fixed by #192

Comments

@Happypig375
Copy link
Contributor

If you accept incorporating nullable reference types then I will start working on an annotation PR 😃

@zwcloud
Copy link
Contributor

zwcloud commented Mar 22, 2020

No. I don't think current Typography's API structures and code quality are good enough to be refactored to enable nullable.

Typography should be refactored thoroughly to clean up APIs first, then start migrate to nullable reference.
Some points:

  • abuse public method, properties: reduce public matters
  • so many temp/middle classes and structs: reduce indirections and unnessary custom structures
  • ambiguous naming: Glyph, GlyphPlan, GlyphSetPosition, GlyphPointF
  • missing unit test: Typography's usefulness is proven through large numbers of visual demos but the lack of unit test is quite painful when one want to refactor: going through visual demos is not a efficient and human-friendly way to test if the refactor modification breaks anything.

@Happypig375
Copy link
Contributor Author

I would say that code quality is independent of nullable annotations.

Nullable annotations require little to no refactors. It is just adding ?s after the types, which just expresses intent of whether nulls are accepted. Therefore, any problems arising from refactoring do not exist.

@zwcloud
Copy link
Contributor

zwcloud commented Mar 22, 2020

@Happypig375 Emm.. I think you can just have a try. There will be ? and ! everywhere and makes nullable reference meaningless, and in turn makes the code not readable.

And if nullable reference warnings are ignored, then there is no point to enable that.

@Happypig375
Copy link
Contributor Author

Normally ! won't be required but I'll try.

@prepare
Copy link
Member

prepare commented Mar 23, 2020

@Happypig375 Happypig375 linked a pull request Mar 25, 2020 that will close this issue
@charlesroddie
Copy link

@zwcloud There will be ? and ! everywhere

@Happypig375 when you implemented this what did you find? How much is safety improved, or is it that only the unsafe places have been identified? Did everything get ?s and !s or was the compiler identifying a lot of safe paths?

@zwcloud @prepare do you still maintain that other cleanup tasks have to be done first? I don't see why that argument holds; these seem independent. Do you have a suggested path to get NRTs in? Is the difficulty the need to test, or that the changes have been too extensive to review?

The official .Net guidance is that NRTs should become ubiquitous with in the .Net 5 timeframe, i.e. by Nov, but of course there are parts of .Net that have historically been slow to update.

@prepare
Copy link
Member

prepare commented Jul 14, 2020

@prepare do you still maintain that other cleanup tasks have to be done first?

Yes, This week task=> Typography.TextServices is under development and refactoring.

and I need to test with these too (#192 (comment))

@Happypig375
Copy link
Contributor Author

Happypig375 commented Jul 14, 2020

when you implemented this what did you find?

Legacy-style projects (non-SDK projects) do not support nullable reference types: dotnet/project-system#5551

I had to include refactors to merge the .NET Framework 2.0 projects with the .NET Standard 2.0 projects into multi-targeting SDK-style projects.

How much is safety improved, or is it that only the unsafe places have been identified?

Yes. However, initialization subroutines called by constructors which initializes fields to non-nullable states were not tracked, leading to the constructor not being recognized as initializing all non-nullable fields. This can be directly solved by MemberNotNullAttribute as specified in dotnet/csharplang#3297 in the future, but modifying these initialization subroutines to be functions instead, and performing assignment in the constructor just makes the logic more clear.

Did everything get ?s and !s or was the compiler identifying a lot of safe paths?

I avoided the use of ! as much as possible because each ! represents a loss of nullable safety. In cases where the nullability of vars are inferred wrongly, spelling out the type usually solves the issue.

@zwcloud
Copy link
Contributor

zwcloud commented Jul 14, 2020

nullable just conflicts with my project's configuration, so I cannot include Typography's source code inside the project directly.
To support that, I will be forced to make unbelievably HUGE effort to add nullable to my project.

Why I need to use Typography's source code? => Because it is not stable enough and often ran into exceptions.

@Happypig375
Copy link
Contributor Author

Just add another project with nullable enabled containing Typography and reference it by your existing project.

@charlesroddie
Copy link

I had to include refactors to merge the .NET Framework 2.0 projects with the .NET Standard 2.0 projects into multi-targeting SDK-style projects.

@prepare I see that not all projects here have been updated to sdk/netstandard2.0. Is the change to netstandard still in progress?

@prepare
Copy link
Member

prepare commented Jul 15, 2020

Yes, not all projects have been updated to sdk/netstandard2.0

I support .net framework 2.0 and netstandard2.0 projects (https://github.com/LayoutFarm/Typography/tree/master/Build).
(But still not in multi-targeting SDK-style projects)

Both set have active maintenance.

@charlesroddie
Copy link

I support .net framework 2.0 and netstandard2.0 projects

That is crazy!

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 a pull request may close this issue.

4 participants