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

proposed 3.0 changes #89

Closed
wants to merge 2 commits into from
Closed

Conversation

nyurik
Copy link

@nyurik nyurik commented Oct 3, 2016

re-posting PR against the v3

@nyurik nyurik mentioned this pull request Oct 3, 2016
@nyurik
Copy link
Author

nyurik commented Oct 3, 2016

I think we should also reformat it to use 4 space tabs, otherwise the diffs are harder to read due to wrapping, but we should do it after we agree on the new structure.

// to make it easier to compare with the previous version

// A list of key-value pairs. Just like Feature's "tags", the repeated pairs
// of integers point to Layer's "keys" (1st int) and "values" (2nd int) array.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we change "1st int" to "odd integers" and "2nd int" to "even integers"?

We should add a note that it's invalid to specify indices that are greater than or equal to the currently parsed index to explicitly disallow circular structures. Parsers are going to implement it that way in order to prevent circular structures.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kkaefer I fixed the odd/even (sorry i missed your comments earlier). Where/how would you like the note to be added?

repeated uint32 object_value = 8 [ packed = true ];

// An array of values. Similar to Feature's "tags", each integer represents
// an index of the Layer's "values" array.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same note about index < current.

@joto
Copy link

joto commented Oct 2, 2018

The version 3 spec will support values of type list or map. But a different encoding is used than in this PR. Closing.

@joto joto closed this Oct 2, 2018
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