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

Transition to using explicit nullable types (#184) #300

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

artog
Copy link

@artog artog commented Oct 24, 2022

✨ What's this?

Removes all warnings related to nullable. Also made a more straight-forward implementation of Maybe<T>

🔗 Relationships

Closes #184

🔍 Why do we want this?

Using explicit nullable is a move towards safer and more stable code.

🏗 How is it done?

Mostly went through all errors and fixes as I went along. Maybe<T> caused a bunch of trouble, so I remade the functionallity using an abstract main class (Maybe<T>) with two deriving classes (Just<T> and Nothing<T>). This separates the intent into two different classes, both simplifying the actual logic and prevents any casting/default issues.

💥 Breaking changes

Hopefully none. Tests are unchanged (and still full of warnings) to allow for insight into backwards compability.

💡 Review hints

Might be wasier to look at commits rather than full PR, especially for the change to Maybe<T>

@artog
Copy link
Author

artog commented Oct 24, 2022

Completed transition for Bearded.Utilities, next up is Bearded.Testing.

@tomrijnbeek
Copy link
Member

Hi artog,

Great to see you've taken an interest in contributing. The nullability issue is one that would be great to close off finally!

I didn't review your PR in detail, since it's still in draft, but the change you made to Maybe caught my eye and I wanted to briefly chime in before you go down a rabbit hole there. A complete reimplementation of Maybe is something I would consider out of scope of just the nullability change. Our implementation is specifically meant as a performance-friendly implementation, which is why we chose for it to be a struct, and not a class. The proposed implementation undermines those principles, so it sounds unlikely that we would adopt this into the library (since there are other implementations that follow the same pattern already).

The Maybe type was built in the time before nullable became widely adopted, so I think its value in existing is debatable in the first place.

My suggestion would be to either revert the Maybe changes, or split them into a separate PR if you feel like you have a strong case for those changes.

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.

Enforce nullable reference types
2 participants