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

Removed InstanceSnapshot snapshot_id's redudant Ref. #730

Merged
merged 1 commit into from
Jul 12, 2023

Conversation

u-train
Copy link
Contributor

@u-train u-train commented Jul 11, 2023

Ref now is an optional inside, so it's redundant to have an option wrapping an option. The only snapshots that were changed were any that had a Ref within (from none to zeroed). Some also had some newlines added in the end.

Aside:
Since Ref is a nullable type, I'm worried about the change. For example, it's easy for someone to insert a null ref into any map where it's expected to be non-null, like in a PatchSet.

This is redundant as Ref is an optional inside, so it's moot...
@Dekkonot Dekkonot added the skip changelog PRs that may skip the changelog enforcement check label Jul 12, 2023
Copy link
Member

@Dekkonot Dekkonot left a comment

Choose a reason for hiding this comment

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

I forgot to add the skip changelog label before running the workflows, sorry!

I'm not too worried about Ref being null-able because it's basically identical to Option<T>. It's something we have to be vigilant for, but no more so than before.

@Dekkonot Dekkonot merged commit 80eb14f into rojo-rbx:master Jul 12, 2023
Dekkonot pushed a commit to UpliftGames/rojo that referenced this pull request Jan 11, 2024
Ref now is an optional inside, so it's redundant to have an option
wrapping an option. The only snapshots that were changed were any that
had a Ref within (from none to zeroed). Some also had some newlines
added in the end.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip changelog PRs that may skip the changelog enforcement check
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants