Skip to content
This repository was archived by the owner on Oct 6, 2022. It is now read-only.

Lmt to be a pointer#27

Merged
mxmCherry merged 3 commits intomxmCherry:masterfrom
evgenigourvitch:master
Oct 12, 2018
Merged

Lmt to be a pointer#27
mxmCherry merged 3 commits intomxmCherry:masterfrom
evgenigourvitch:master

Conversation

@evgenigourvitch
Copy link
Contributor

to allow to pass 0 values when required.

@mxmCherry
Copy link
Owner

mxmCherry commented Oct 11, 2018

Mmm, does it make sense to have this as tri-state (null/absent, 0, 1)?
It is recommended in spec.
And "absence" treatment is not described.
And I cannot think that absence would ever be treated by anyone in any special way.
Probably, there's no need for it to be able to become absent?

Thinking about removing the omitempty modifier (so always pass 0 or 1), just not sure if/how it may affect consumers.

Though, it's not "required", just "recommended"...

@mxmCherry
Copy link
Owner

@evgenigourvitch could you, please, check if you need the same for DNT attr (the one above Lmt)?
If yes - let's do it in one go.
If not - just leave it (don't change what's not needed/requested).

I think that it makes sense to adhere to such rules:

  • "required" in spec -> no "omitempty"
  • not "required" in spec - "omitempty" and make it pointer if zero value has some special meaning

Just to make such decisions simple in future.

@evgenigourvitch
Copy link
Contributor Author

Regarding the DNT - currently it's fine for me, but "tomorrow" it could be changed and I'll need to send 0 value, so lets do it in one go, just like you said. Changing it.

IMHO, the omitempty modifier is just great! It allows to save traffic costs very easily and believe in LARGE scale its worth it - every byte counts.

I think pointer is a nice solution for tri-state values null/0/1. Absence (null) will easily allow to save traffic costs in most of the cases. And 0/1 will be easily passed when needed.
Sounds reasonable?

@mxmCherry
Copy link
Owner

Will merge / update README with "omitempty" usage hints / make a new major ver release today in ~ 8-9 hours.

@evgenigourvitch
Copy link
Contributor Author

@mxmCherry thanks a lot!

@mxmCherry mxmCherry added this to the v11.0.0 milestone Oct 12, 2018
@mxmCherry mxmCherry merged commit 7600f5c into mxmCherry:master Oct 12, 2018
@mxmCherry
Copy link
Owner

@evgenigourvitch released as https://github.com/mxmCherry/openrtb/releases/tag/v11.0.0 , thanks for contributing!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants