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.

Sign up for GitHub

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

Jump to bottom

Use @mapbox/gazetteer for benchmark coordinates #8063

Merged
merged 6 commits into from
Mar 25, 2019
Merged

Use @mapbox/gazetteer for benchmark coordinates #8063

merged 6 commits into from
Mar 25, 2019

Conversation

tristen
Copy link
Member

@tristen tristen commented Mar 21, 2019

Replace bench/lib/style_locations.js with the same place data made available in @mapbox/gazetteer.

@ansis thanks for the tip to use MercatorCoordinate.fromLngLat to derive tile index. Could I get you to review this?

Closes #8059

@tristen tristen requested a review from ansis March 21, 2019 18:32
This was referenced Mar 21, 2019
Closed
Closed
@tristen
Copy link
Member Author

tristen commented Mar 21, 2019

🤔 It's not clear to me why tests are failing.

mourner
mourner requested changes Mar 21, 2019
View reviewed changes
Copy link
Member

@mourner mourner left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The bench changes look good (aside from the test failures), but did you accidentally ran yarn upgrade? The amount of changes in yarn.lock is not expected. This might be the cause of the test failures.

@tristen
Copy link
Member Author

tristen commented Mar 21, 2019 •
edited
Loading

The bench changes look good (aside from the test failures), but did you accidentally ran yarn upgrade?

@mourner hmm I don't think I did? I just tried blowing away node_modules/ + yarn.lock, running yarn again and I don't see any difference against the lock file :/

@ryanhamley
Copy link
Contributor

You may need to just update Yarn. That fixed this issue for me.

@tristen
Copy link
Member Author

tristen commented Mar 21, 2019

You may need to just update Yarn. That fixed this issue for me.

@ryanhamley that seemed promising but there are still no changes against yarn.lock after upgrading yarn to 1.15.2.

@mourner
Copy link
Member

mourner commented Mar 21, 2019

@tristen do not delete the yarn.lock file. Run yarn when it's reset to its master state.

mourner
mourner reviewed Mar 21, 2019
View reviewed changes
bench/lib/style_locations.js Outdated
@@ -1,3 +1,5 @@
/* eslint camelcase: 0 */
Copy link
Member

Choose a reason for hiding this comment

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

Just do feature.properties['place_name'] instead.

@tristen tristen requested review from mourner and removed request for ansis March 21, 2019 20:29
mourner
mourner reviewed Mar 21, 2019
View reviewed changes
package.json Outdated
@@ -15,6 +15,7 @@
"dependencies": {
"@mapbox/appropriate-images-react": "^1.0.0",
"@mapbox/dr-ui": "0.6.0",
"@mapbox/gazetteer": "^3.1.0",
Copy link
Member

Choose a reason for hiding this comment

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

Needs to be in devDeps.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oy ... me and yarn today :/

@tristen tristen requested a review from mourner March 22, 2019 14:10
@mourner mourner merged commit 182874c into master Mar 25, 2019
@mourner mourner deleted the issue-8059 branch March 25, 2019 08:50
mourner added a commit that referenced this pull request Apr 25, 2019
@mourner
Merge branch 'release-liquid' into publisher-production
353dc34 
* release-liquid: (84 commits)
  v0.54.0 (take two) (#8184)
  Fix disappearing controls in Safari 12+ (#8193) (#8194)
  [docs] token refactor in docs-page-shell (#8174) (#8181)
  v0.54.0-beta.2 (#8166)
  Bugfix - removeFeatureState fails with target.id === 0 (#8150) (#8164)
  update one more frame after canvas source paused (#8130) (#8163)
  move docs dependencies to dev (#8121) (#8129)
  v0.54.0 (#8115)
  CP removeFeatureState fix (#8090)
  v0.54.0-beta.1 (#8084)
  Allow setStyle diff by default with localIdeographFontFamily on (#8081)
  Upgrade various (mostly dev) deps, downgrade GL (#8078)
  Fix default marker positioning (#8074)
  Use @mapbox/gazetteer for benchmark coordinates (#8063)
  bump react-dom to v16.3.3 to fix minor vulnerability warning
  Fix and refactor the benchmark suite (#8066)
  Add max width for popups (#7906)
  Extrusions: Do not try to triangulate non-polygon type features (#7685)
  docs fixes after the merge
  limit flow max_workers and upgrade to v0.95.1 (#8061)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mourner mourner mourner approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tristen @ryanhamley @mourner