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

feat(OpenApiType): Warn about non-required nullable properties #194

Merged

Conversation

provokateurin
Copy link
Member

Closes #177

@provokateurin provokateurin merged commit 40302d1 into main Dec 17, 2024
10 of 15 checks passed
@provokateurin provokateurin deleted the feat/openapitype/warn-non-required-nullable-properties branch December 17, 2024 16:17
@nickvergessen
Copy link
Member

nickvergessen commented Dec 18, 2024

Warning: Response definitions: TalkChatMentionSuggestion: Property "statusClearAt" is both nullable and not required. Please consider only using one of these at once.
Warning: Response definitions: TalkChatMentionSuggestion: Property "statusIcon" is both nullable and not required. Please consider only using one of these at once.
Warning: Response definitions: TalkChatMentionSuggestion: Property "statusMessage" is both nullable and not required. Please consider only using one of these at once.

🤷 What problem that does not exist are we trying to fix?
A user status consists of status, message, icon, clearAt, … All but the status are nullable.
Feels weird to me that we have to crash the API and in all clients have to check if each attribute of the status is actually available?

Warning: Response definitions: TalkParticipant: Property "phoneNumber" is both nullable and not required. Please consider only using one of these at once.
Warning: Response definitions: TalkParticipant: Property "callId" is both nullable and not required. Please consider only using one of these at once.

Similarly in Talk we use the same ResponseDefinition for normal and moderator users, so everything that only moderators can see is "not required". But they can still be null in those cases

@provokateurin
Copy link
Member Author

What problem that does not exist are we trying to fix?

I consider it bad design, because these are two different concepts to say a value is not present. In Javascript you can see this perfectly, because these fields could have the value null or undefined and you'd have to check against both.

Feels weird to me that we have to crash the API

Crash? This is only a warning you can ignore without consequences.

in all clients have to check if each attribute of the status is actually available?

Depends on how the clients handle the data. Some will handle these cases the exact same way (by using null as the value), some like Javascript are better due to the problem I outlined above.

Similarly in Talk we use the same ResponseDefinition for normal and moderator users, so everything that only moderators can see is "not required". But they can still be null in those cases

No problem with that. You can just keep it that way. I personally would only use null for this case as the differentiation if a user is a moderator or not shouldn't be based on the availability of some properties.

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.

Suggest to make object properties either optional or nullable, but not both at once
3 participants