Skip to content
This repository has been archived by the owner on Apr 10, 2018. It is now read-only.

Document enum values individually #497

Closed
jfirebaugh opened this issue Sep 19, 2016 · 13 comments
Closed

Document enum values individually #497

jfirebaugh opened this issue Sep 19, 2016 · 13 comments
Assignees

Comments

@jfirebaugh
Copy link
Contributor

Change:

    "icon-rotation-alignment": {
      "values": [
        "map",
        "viewport",
        "auto"
      ],
      "default": "auto",
      "doc": "Orientation of icon when map is rotated. Aligns icon to the plane of the viewport when set to `viewport` or the plane of the map when set to `map`. Selecting `auto` defaults to `map` for line placement and `viewport` for symbol placement",

To:

    "icon-rotation-alignment": {
      "values": {
        "map": {
          "doc": "Icons are oriented north-up, regardless of map rotation."
        },
        "viewport": {
          "doc": "Icons are oriented such that they are always upright on screen."
        },
        "auto": {
          "doc": "Equivalent to `map` if this layer uses `"symbol-placement": "line"`, otherwise `viewport`."
        }
      },
      "default": "auto",
      "doc": "Determines the orientation behavior of icons when the map is rotated.",

Then the generated docs can list the enum values with their documentation in a definition list.

@1ec5
Copy link
Contributor

1ec5 commented Sep 19, 2016

This is a blocker for mapbox/mapbox-gl-native#5953, which is blocking the iOS SDK v3.4.0 release. (The Objective-C enum values need to be individually documented; otherwise, the symbols are entirely omitted from documentation.)

@incanus
Copy link
Contributor

incanus commented Sep 28, 2016

Taking a look at this.

@incanus incanus self-assigned this Sep 28, 2016
@jfirebaugh
Copy link
Contributor Author

The places I know that are downstream users of "values" and will need to be updated in sync with this change:

@samanpwbb
Copy link
Contributor

Mapbox studio will need to be updated based on this change well.

@1ec5
Copy link
Contributor

1ec5 commented Sep 28, 2016

@incanus
Copy link
Contributor

incanus commented Sep 29, 2016

Mapbox studio will need to be updated based on this change well.

@samanpwbb Is this something I should start looking into, or is it easier for someone already in Studio to handle?

@samanpwbb
Copy link
Contributor

Studio team can take care of this when we upgrade the style spec version. Just something we'll need to be aware of. Worth ticketing downstream.

@scothis
Copy link
Contributor

scothis commented Sep 30, 2016

As proposed this is a breaking change to the style spec, with all the associated 😭. The same content can be added in a non-breaking way:

"icon-rotation-alignment": {
  "values": [
    "map",
    "viewport",
    "auto"
  ],
  "default": "auto",
  "doc": "Orientation of icon when map is rotated. Aligns icon to the plane of the viewport when set to `viewport` or the plane of the map when set to `map`. Selecting `auto` defaults to `map` for line placement and `viewport` for symbol placement",
  "values-meta": {
    "map": {
      "doc": "Icons are oriented north-up, regardless of map rotation."
    },
    "viewport": {
      "doc": "Icons are oriented such that they are always upright on screen."
    },
    "auto": {
      "doc": "Equivalent to `map` if this layer uses `"symbol-placement": "line"`, otherwise `viewport`."
    }
  }
}

@jfirebaugh
Copy link
Contributor Author

@scothis We've already put the work into updating the places that need updating in this repository and mapbox-gl-native. Unless it's going to be a huge amount of work to adjust Studio, I'd like to continue with the change as implemented.

@scothis
Copy link
Contributor

scothis commented Sep 30, 2016

@jfirebaugh I'm not worried about Studio, it's the downstream users we're not aware of. If you're comfortable making a breaking change without bumping the major version number, just be aware of what you're doing. We don't advertise that the project follows semver, but it's certainly the expectation.

@1ec5
Copy link
Contributor

1ec5 commented Sep 30, 2016

My understanding – correct me if I’m wrong – is that the version number tracks the content of the style specification, not any particular representation of it. There are a number of tools in this repository that could be individually versioned if we chose to, such as diff.js or indeed the JSON-formatted representation.

@incanus
Copy link
Contributor

incanus commented Sep 30, 2016

Didn't mean to cut off discussion here. #510 needed to happen regardless. We can reopen here or continue elsewhere the versioning discussion fallout.

@davidtheclark
Copy link
Contributor

I think this PR brings to the fore (but is not the first to introduce) a question about the intended purpose of the affected file. Automatic documentation generation is one thing; a JSON schema precisely describing the data structure is another. A JSON file that is optimal for one of these purposes might very well not be very good for the other. I wonder if it wouldn't be a good idea to create separate files to serve these separate ends. Or if not that, we might want to reconsider the JSON structure more generally to allow for changes like this one in the future without breaking things.

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

No branches or pull requests

6 participants