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

Cleanup utility projects #517

Merged
merged 11 commits into from
Aug 8, 2023

Conversation

lahm86
Copy link
Collaborator

@lahm86 lahm86 commented Aug 7, 2023

I'll group projects together into separate cleanup PRs to try to keep the file numbers a bit more manageable.

This one includes the 5 utility projects - each cleanup action is in its own commit.

Part of #425.

@lahm86 lahm86 added the internal label Aug 7, 2023
@lahm86 lahm86 added this to the 1.8.0 milestone Aug 7, 2023
@lahm86 lahm86 self-assigned this Aug 7, 2023
@lahm86 lahm86 force-pushed the issue-425-net6-cleanup-stage1 branch from 9deb956 to 53f24c1 Compare August 7, 2023 20:45

public static TR1Level? CurrentLevelAsTR1
public static TR1Level CurrentLevelAsTR1
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, why are we removing the ?? This function can still return null.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's currently only called in the likes of the situation below, so we still check for null at that end. I find this approach cleaner, the VS warnings when nullable is on are less than helpful I find.

https://github.com/LostArtefacts/TR-Rando/blob/master/TRLevelToolset/Controls/DataControls/TR/TRAnimatedTextureControl.cs#L19

This is the UI when the level is null.

image

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally think it's cleaner to leave these intact. The warnings are not wrong, even if they're annoying.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fair. I'll re-enable and look at clearing the warnings.

@lahm86 lahm86 requested a review from rr- August 8, 2023 08:38
@lahm86 lahm86 merged commit 61a77bb into LostArtefacts:master Aug 8, 2023
2 checks passed
@lahm86 lahm86 deleted the issue-425-net6-cleanup-stage1 branch August 8, 2023 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants