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

attribution, name, tilejson, version, description #37

Merged
merged 2 commits into from
May 8, 2018
Merged

Conversation

mapsam
Copy link
Contributor

@mapsam mapsam commented May 2, 2018

Starting to add some default language from the previous specification for attribution, name, tilejson, version, and description. Planning to leave questions inline for discussions!

cc @GretaCB

"name": "Earthsea v2"
}
```

## 3.11 `scheme`
## 3.12 `template`
## 3.13 `tilejson`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@GretaCB in the previous version we mention the tilejson value being semver, but our new 3.0 spec only defines major.minor and no patch. Should we rephrase this?

Copy link
Contributor

Choose a reason for hiding this comment

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

@mapsam Good eye. I'm thinking we should change this and make it super specific when to bump minor and when to bump major. For example, fixing insignificant typos/spelling errors wouldnt need any kind of version bump. But if a change affects implementation in anyway (like the spelling of a tilejson property), that would require a major bump.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice - I agree. We also mention when major.minor in the CONTRIBUTING.md which we wrote recently, which appears to nicely match your example.

"tilejson": "3.0"
}
```

## 3.14 `tiles`
## 3.15 `vector_layers`
## 3.16 `version`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A few notes/questions for the version field:

  • added a link to semver.org
  • updated the first sentence to include "... representing the vector tiles themselves." to be more explicit. Thoughts?
  • Added example text (in bold) to the following sentence: "When changes across tiles are introduced, such as polygon or linestring geometry updates, the minor version MUST change."
  • Question: The final sentence discussing major versions is confusing to me... "Implementations MUST NOT use tiles with different major versions." what does this actually mean? Can we provide an example to make it more clear?

Copy link
Contributor

Choose a reason for hiding this comment

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

"Implementations MUST NOT use tiles with different major versions." what does this actually mean? Can we provide an example to make it more clear?

Same. Perhaps it's saying, any implementation must ensure its tilejson major version is consistent. If updating tilejson major version, it must apply to all uses of tilejson throughout the system.

...?

Copy link
Contributor

Choose a reason for hiding this comment

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

updated the first sentence to include "... representing the vector tiles themselves." to be more explicit. Thoughts?

From what I understand, the version field here is referring to the tilejson version, not the vector-tile version.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh pardon, the tilejson field is referring to the tilejson version.

The version field is referring to the vector-tile version. Therefore, your addition sounds great. I think also adding something like:

A semver.org style version number of the tiles

Also, can we assume it's always vector-tiles?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, can we assume it's always vector-tiles?

Good point, I don't think we can. In fact now that I re-read my added example text "such as polygon or linestring geometry updates" I'm wondering if this isn't actually true, and is more related to breaking changes of the underlying data. At least for vector tiles, this would be something like "changing the name of layerA to layer1" which would break downstream tools referencing the data. I'm not exactly sure how you'd version raster data though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the "major" example to layer name changes, and just say "tiles" instead of "vector tiles" since these could be raster tiles.

@mapsam mapsam changed the title name, tilejson, version, description attribution, name, tilejson, version, description May 2, 2018
@mapsam mapsam mentioned this pull request May 2, 2018
41 tasks
@@ -39,28 +39,76 @@ Implementations MUST treat unknown keys as if they weren't present. However, imp

## 3.1 `attribution`

REQUIRED|OPTIONAL (description of dependents/dependencies)
OPTIONAL. Default: null.
Copy link
Contributor

Choose a reason for hiding this comment

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

“Default” is … If you are consuming TileJSON and this field does not exist, you should assume the value is X

@mapsam what are your thoughts on making this explicit somewhere? Perhaps part of Structure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good!

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Just added

Implementations MUST treat invalid values for keys as if they weren’t present. If the the field is an optional field and the value is invalid, the default value MAY be applied.

MAY could be a SHOULD or a MUST

Copy link
Contributor

Choose a reason for hiding this comment

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

Since these fields are Optional, going to stick with MAY

MAY This word, or the adjective "OPTIONAL", mean that an item is
truly optional. ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome - thanks @GretaCB 👍

This was referenced May 2, 2018
@mapsam
Copy link
Contributor Author

mapsam commented May 8, 2018

Per discussion with @GretaCB - merging this into 3.0 base so we can sync up some other branches!

@mapsam mapsam merged commit 0f99d1a into 3.0 May 8, 2018
@mapsam mapsam deleted the 3.0-some-defaults branch May 8, 2018 18:56
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.

2 participants