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

Add api-supported-mode to spec.validate #8663

Merged
merged 7 commits into from
Sep 25, 2019
Merged

Conversation

samanpwbb
Copy link
Contributor

@samanpwbb samanpwbb commented Aug 20, 2019

The style API has stricter stylesheet validation than Mapbox GL JS. This PR adds a new mode to the validator that will inform the user if their style is fit to be uploaded to the style API. The purpose of this PR is to add transparency to the style API and unify test suites for both validation steps.

Changelog suggestion:

  • Add a new validation function that returns the same error responses as the Mapbox Styles API. Use as a CLI tool with gl-style-validate --api-supported-mode. Use in Javascript by importing validateMapboxApiSupported from the @mapbox/mapbox-gl-style-spec package.

Launch Checklist

  • briefly describe the changes in this PR
  • Finish the code
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page
  • tagged @mapbox/studio and/or @mapbox/maps-design if this PR includes style spec changes
  • tagged @mapbox/gl-native if this PR includes shader changes or needs a native port

Copy link
Contributor

@asheemmamoowala asheemmamoowala left a comment

Choose a reason for hiding this comment

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

@samanpwbb The approach taken here looks good to me.

src/style-spec/validate/validate.js Outdated Show resolved Hide resolved
src/style-spec/validate/validate_mapbox_api_supported.js Outdated Show resolved Hide resolved
@samanpwbb samanpwbb force-pushed the api-supported-mode branch 2 times, most recently from 59420dd to 1e82b99 Compare September 24, 2019 20:36
@samanpwbb samanpwbb marked this pull request as ready for review September 24, 2019 20:52
@samanpwbb samanpwbb requested a review from jseppi September 24, 2019 20:52
@samanpwbb
Copy link
Contributor Author

samanpwbb commented Sep 24, 2019

Okay, this is ready for review. I opted for creating a validation function that is fully independent of the existing recursive validation function. The existing system highly depends on the style-spec config file, while my new validator is a little more imperative.

I considered writing api-style-supported mode as a new version of spec instead (or rather, as a set of overrides that would be merged into the main spec before tests run). I think this would be cool, but realized that it add quite a few new challenges. For example, I would need to add new pattern-matching validators to the style spec, so I backed out and kept things simple.

I would love a review from @jseppi in addition to a review from the gl team, since this PR significantly impacts upstream work in api-styles.

cc: @mapbox/map-design-team 😄

@samanpwbb
Copy link
Contributor Author

Oh one more thing: How am I supposed to test the function in style-spec/bin ? Trying to run them directly didn't work :|

Copy link
Contributor

@arindam1993 arindam1993 left a comment

Choose a reason for hiding this comment

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

LGTM
Minor nitpick, also more of a question, any reason to use .forEach instead of for..of?

@samanpwbb
Copy link
Contributor Author

Minor nitpick, also more of a question, any reason to use .forEach instead of for..of?

Here: https://github.com/mapbox/mapbox-gl-js/pull/8663/files#diff-abf4b5c9003a59059bbce3d73ab97182R65 – I need index to report a nice key for the error, and forEach is a little nicer than tracking an i variable. Since I was using forEach there I stuck with it. I wasn't able to discern from GL's codebase a preferred style for iterating over objects but I'd be happy to switch to for..of if that's more inline with the direction you all are headed in.

Copy link
Contributor

@jseppi jseppi left a comment

Choose a reason for hiding this comment

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

Generally I think this looks really good, @samanpwbb!

A few nits and small questions that I think you can just clean-up or decide not to address.

src/style-spec/bin/gl-style-validate Outdated Show resolved Hide resolved
src/style-spec/read_style.js Outdated Show resolved Hide resolved
src/style-spec/read_style.js Outdated Show resolved Hide resolved
src/style-spec/validate_mapbox_api_supported.js Outdated Show resolved Hide resolved
src/style-spec/validate_mapbox_api_supported.js Outdated Show resolved Hide resolved
src/style-spec/validate_style.min.js Show resolved Hide resolved
Copy link
Contributor

@asheemmamoowala asheemmamoowala left a comment

Choose a reason for hiding this comment

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

Thank you for adding the tests. They are 💯

@samanpwbb samanpwbb merged commit fe621bb into master Sep 25, 2019
@samanpwbb samanpwbb deleted the api-supported-mode branch September 25, 2019 22:25
samanpwbb added a commit that referenced this pull request Sep 25, 2019
The Mapbox Styles API uses stricter stylesheet validation than Mapbox GL JS. This commit adds a new function to the style spec and a new option to the mapbox-gl-validate CLI tool that will inform the user if their style is fit to be uploaded to the Mapbox style API.
@kkaefer
Copy link
Member

kkaefer commented Sep 26, 2019

@samanpwbb could you please draft a note for our changelog?

@samanpwbb
Copy link
Contributor Author

Here:

  • Add a new validation function that returns the same error responses as the Mapbox Styles API. Use as a CLI tool with gl-style-validate --api-supported-mode. Use in Javascript by importing validateMapboxApiSupported from the @mapbox/mapbox-gl-style-spec package.

If that’s too long, just use the first sentence

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.

5 participants