Skip to content

Conversation

@Fokko
Copy link
Contributor

@Fokko Fokko commented Jul 26, 2022

The spec allows us to set the properties to null, but instead, we can just send the default empty dict {} if we don't want to set any properties at all.

The spec allows us to set the properties to `null`, 
but instead, we can just send the default empty dict `{}` 
if we don't want to set any properties at all.
@rdblue
Copy link
Contributor

rdblue commented Jul 26, 2022

I think we want to have as few requirements as possible, right? Is there value in doing this?

@kbendick
Copy link
Contributor

Small correction: Right now, as the field is not marked as required, implementors can skip sending the field entirely (as opposed to only having the option to send null).

We tend to do that within some of the *Parser.toJson functions in Java for null (e.g. not set the field).

There's nothing stopping implementors from sending the empty dict now, but the Java code at least defensively returns an empty ImmutableMap in case it parses null:

public Map<String, String> properties() {
return properties != null ? properties : ImmutableMap.of();
}

Is there a benefit on the Python side to this requirement?

@Fokko
Copy link
Contributor Author

Fokko commented Jul 26, 2022

It is a nit here: #5287 (comment) It is about just always sending {} instead of omitting the field.

@rdblue
Copy link
Contributor

rdblue commented Jul 26, 2022

I think it makes sense for Python to not deal with None and to always send {}, but I wouldn't change the spec to require that. The spec is permissive because we expect people to make mistakes and we want service implementations to be able to accept requests that omit properties or set it to null. But we want to be as consistent as possible when producing requests. That's why I would default it in Python and always send it, but not update the spec.

@Fokko
Copy link
Contributor Author

Fokko commented Jul 26, 2022

Got it. The reason I have in Python like that is that I've generated the code from the spec itself. If this is something that we prefer, then I think it makes sense to update the spec itself, so future generators have this as well. I don't think we need to update the Java code for example because we can always be more liberal when parsing this.

@Fokko Fokko closed this Jul 26, 2022
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.

3 participants