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 nullability for Cell and co. and clean up #3880

Merged
merged 1 commit into from
May 28, 2024
Merged

Conversation

YoshiRulz
Copy link
Member

@YoshiRulz YoshiRulz commented Mar 27, 2024

Starting on #3869 (comment). This builds upon that PR and the subsequent commits through to d67b1c1, mainly 4f144c7.

@Morilli
Copy link
Collaborator

Morilli commented Mar 31, 2024

I do wonder, is this even the correct direction to go? As in, you made Column and RowIndex explicitly nullable, but could we make them explicitly not-null? What's even the realistic usecases for constructing a (non-null) Cell object with either null RowIndex or null Column?

If either of those (or both) could be non-null, it would most likely simplify the code.

@YoshiRulz
Copy link
Member Author

I agree, but in practice they both are set to null sometimes, for example:

CurrentCell = new Cell
{
Column = null,
RowIndex = null
};

@Morilli
Copy link
Collaborator

Morilli commented Mar 31, 2024

Yeah I've seen those cases in the code, but this one for example could just be replaced with CurrentCell = null, right?

@YoshiRulz
Copy link
Member Author

¯\_(ツ)_/¯

@YoshiRulz
Copy link
Member Author

I think this should be merged, and if the simplification discussed above is possible, it can be done later.

@YoshiRulz YoshiRulz merged commit 9eb9ffa into master May 28, 2024
1 check passed
@YoshiRulz YoshiRulz deleted the inputroll-nrts branch May 28, 2024 20:24
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.

None yet

2 participants