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

OPTIONAL field MAY be null #351

Merged
merged 1 commit into from
Oct 7, 2016

Conversation

coolljt0725
Copy link
Member

This field MAY be null is duplicate with the OPTIONAL

Signed-off-by: Lei Jitang leijitang@huawei.com

@jonboulle
Copy link
Contributor

I think this was talked about in a previous issue somewhere - it's not quite redundant - the field being optional means it might not be present, but that's different to it being present but having an empty/null value.

@jonboulle
Copy link
Contributor

Could probably do with a once-over of the spec to make sure we're consistent about this for all fields.

@coolljt0725
Copy link
Member Author

Could probably do with a once-over of the spec to make sure we're consistent about this for all fields.

+1 for this to make it more clear.

@coolljt0725 coolljt0725 reopened this Sep 30, 2016
@coolljt0725 coolljt0725 force-pushed the remove_dup branch 2 times, most recently from cdac091 to 09639ce Compare September 30, 2016 09:41
@coolljt0725 coolljt0725 changed the title Remove duplicate clarification OPTIONAL field MAY be null Sep 30, 2016
@coolljt0725
Copy link
Member Author

@jonboulle add a sentence to clarify this

@jonboulle
Copy link
Contributor

@coolljt0725 how is this clearer than what was originally there? it's no longer in context

@coolljt0725
Copy link
Member Author

@jonboulle maybe I just misunderstood your point. I think all the OPTIONAL field could be null

@jonboulle
Copy link
Contributor

That makes sense, but if so, I'd rather move it to the top of the
PROPERTIES section, and tweak the wording a bit -

Any OPTIONAL field MAY also be set to null, which is equivalent to being
unspecified.

On 30 September 2016 at 11:44, Lei Jitang notifications@github.com wrote:

@jonboulle https://github.com/jonboulle add a sentence to clarify this


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#351 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACewN4PCFQsY_7yZJBlg6dP3cVkW05yHks5qvNnvgaJpZM4KK3yA
.

@duglin
Copy link
Collaborator

duglin commented Sep 30, 2016

To be be really anal:

Any OPTIONAL field MAY also be set to null, which is equivalent to being absent.

@wking
Copy link
Contributor

wking commented Sep 30, 2016

On Fri, Sep 30, 2016 at 07:39:34AM -0700, Jonathan Boulle wrote:

That makes sense, but if so, I'd rather move it to the top of the
PROPERTIES section, and tweak the wording a bit -

If we can make a general rule of this (and that sounds good to me), I
think that rule belongs in the README or some other file to cover all
of our JSON, instead of making a rule that applies only to
application/vnd.oci.image.config.v1+json.

Any OPTIONAL field MAY also be set to null, which is equivalent to
being unspecified.

I like this wording^ [edit: @duglin's wording 0 is better]. I've floated using ‘null’ (vs. leaving the
property off) as a way to explicitly set a property unlimited 1, but
that could be tedious to handle in Go and the idea hasn't caught on
yet. @crosbymichael has also pushed for a distinction between ‘null’
and Go's default value 2; I'm not clear on why, but it may impact
this as well.

Signed-off-by: Lei Jitang <leijitang@huawei.com>
@vbatts
Copy link
Member

vbatts commented Oct 6, 2016

LGTM

Approved with PullApprove

1 similar comment
@philips
Copy link
Contributor

philips commented Oct 7, 2016

LGTM

Approved with PullApprove

@jonboulle jonboulle merged commit 756744a into opencontainers:master Oct 7, 2016
@coolljt0725 coolljt0725 deleted the remove_dup branch October 8, 2016 01:52
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.

6 participants