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

fix(instance): specify that roleRestricted is nullable #324

Closed

Conversation

ttshivers
Copy link
Contributor

Specify that roleRestricted is nullable as detected through observation.

Fixes: #323

Specify that roleRestricted is nullable as detected through observation.

Fixes: vrchatapi#323
@Rexios80
Copy link
Collaborator

Hold on if the field isn't in the required section it should generate as nullable already. Maybe my SDK is just out of date if someone already removed it from the required section.

@Rexios80
Copy link
Collaborator

Yeah that's definitely what happened. Thank you for the PR, but this is not a necessary change since the roleRestricted is already not in the required section. My bad for not using the latest version of the spec.

@Rexios80 Rexios80 closed this Apr 29, 2024
@ariesclark
Copy link
Member

To confirm, the lack of a property doesn't mean it can't be null, { "foo": null } is nullable, while {} (without foo) is optional (not in required property list). If it can exist at all, in a nullish state, it should be marked as nullable.

@ttshivers ttshivers deleted the instance-roleRestricted-nullable branch April 29, 2024 20:00
@Rexios80
Copy link
Collaborator

The exception I pasted in that issue was that the key was not in the json, not that the key was there but the value was null

@Rexios80
Copy link
Collaborator

Also I'm not sure any language can differentiate between a missing key and a null value besides throwing an exception if the key is missing which is probably never what we want. I'm almost thinking we should not use the required section at all and just mark fields as nullable instead, but that would be a large refactor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Instance.roleRestricted is nullable
3 participants