specs-go/v1/descriptor: Drop 'omitempty' from mediaType

[wking]

This is a required property, so omitempty serves no purpose.

[philips]

LGTM

[stevvooe]

omitempty has no bearing on whether the field is required or not. They are completely unrelated. It just means that if there is an empty string, you don't emit the field. Either way, the descriptor is still invalid.

I am in favor of leaving omitempty in place for all fields.

[wking]

On Fri, Feb 03, 2017 at 02:28:12PM -0800, Stephen Day wrote: It just means that if there is an empty string, you don't emit the field. Either way, the descriptor is still invalid.

Agreed.

I am in favor of leaving `omitempty` in place for all fields.

Your “either way, the descriptor is still invalid” argument will break down if there is a required property where Go's zero-value is legal. For example, Descriptor.Size may be zero (and it's not omitempty [1]). I don't see a point to having ‘omitempty’ where you never intend it to be used, but *shrug*, it's your project. [1]: https://github.com/opencontainers/image-spec/blob/ed18de10b0d7e34076e1557bdcd6f500504a6673/specs-go/v1/descriptor.go#L29

[stevvooe]

@wking Look, I am just trying to reduce churn in areas that don't need changes. How bout someone tries to tackle action problems?

#467
#415

This doesn't even address a real problem. Is there a bug? Did this break something? No, it is just a waste of time.

For example, Descriptor.Size may be zero (and it's not omitempty [1]).

What exactly is the point of a zero-sized target object? In fact, there can only be one valid item with zero-sized target and that Digest is sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855. What, then, is the media type of said thing?

Let's get focused on a 1.0 and stop faffing about.

[wking]

On Fri, Feb 03, 2017 at 03:44:23PM -0800, Stephen Day wrote: This doesn't even address a real problem. Is there a bug? Did this break something? No, it is just a waste of time.

This is minor polishing to reduce confusion like [1] and save maintainers time in the long run (by not having to field PRs like #547).

What exactly is the point of a zero-sized target object?

I dunno, but nobody feels strongly enough about it to make it illegal (and I think not jumping through hoops to make things illegal is a good thing).

Let's get focused on a 1.0 and stop faffing about.

I didn't expect this removal to be so contentious ;). But I'll PR some language for #467 to rebalance my karma (although it's not milestoned 1.0.0, perhaps an oversight?). [1]: #547 (comment)

[stevvooe]

This is minor polishing to reduce confusion like [1] and save
maintainers time in the long run (by not having to field PRs like
#547).

How is this the cause of #547? The problem with #547 exists between a keyboard and a chair.

I didn't expect this removal to be so contentious ;).

It is just unnecessary.

[wking]

On Fri, Feb 03, 2017 at 04:43:04PM -0800, Stephen Day wrote: > This is minor polishing to reduce confusion like [1] and save > maintainers time in the long run (by not having to field PRs like > #547). How is this the cause of #547? The problem with #547 exists between a keyboard and a chair.

Excepting repository curruption and similar, *all* problems are between a keyboard and a chair ;). For more details on the #547 case in particular, see this comment [a] (which I also linked earlier [b]).

> I didn't expect this removal to be so contentious ;). It is just unnecessary.

*This same change* landed without contention in #410 (for the since-removed Versioned.MediaType, which was also required at that point [1]). There was backing discussion around that change in [2]. Looking back at the history of Descriptor.MediaType, we have it landing without omitempty in #172, and the omitempty getting injected without discussion in #411. I'm not sure what @vbatts' motivation was for adding it; maybe he can weigh in here? And grepping master for ‘omitempty’ shows Descriptor.MediaType as the *only* omitempty property that is REQUIRED in the associated Markdown (all of the other omitempty properties are explicitly OPTIONAL). So is this PR going to have a big impact on compiled Go code? No. But I don't think I had reason to expect this PR would have taken any more maintainer time than a glance at the diff, an LGTM in a comment, and a tap on the merge button. [a]: #547 (comment) [b]: #553 (comment) [1]: https://github.com/opencontainers/image-spec/pull/410/files#diff-ba085f9a0c64bb6a91f374aa38b15393L25 [2]: 4267a18#commitcomment-19497879

[stevvooe]

@wking Unfortunately, had I seen #410, I would have rejected it.

omitempty ensures that we have clean output that reduces errors in consumers. omitempty has nothing to do with REQUIRED vs OPTIONAL. Stop making that false equivalence.

I am closing this and I hope someone will take the time to revert #410 so I don't have to.