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

Fixes bugs in findStopLessThanOrEqualTo algorithm #8134

Merged
merged 9 commits into from
Apr 11, 2019
Merged

Fixes bugs in findStopLessThanOrEqualTo algorithm #8134

merged 9 commits into from
Apr 11, 2019

Conversation

Mike96Angelo
Copy link
Contributor

@Mike96Angelo Mike96Angelo commented Apr 8, 2019 •
edited
Loading

Old behaviour

  • When the input > all stops it returned the second-last stop, should have been the last stop.
findStopLessThanOrEqualTo([0, 1, 2, 3, 4, 5, 6, 7], 8) // 6 but should have been 7
  • When more than one stop had the same value it didn't always return the last stop
findStopLessThanOrEqualTo([0.5, 0.5], 0.5) // 0 but should have been 1

New behaviour

  • When the input > all stops it returns the last stop.
findStopLessThanOrEqualTo([0, 1, 2, 3, 4, 5, 6, 7], 8) // 7
  • When more than one stop has the same value it always returns the last stop
findStopLessThanOrEqualTo([0.5, 0.5], 0.5) // 1

Launch Checklist

  • briefly describe the changes in this PR
  • 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

Fixes bugs in findStopLessThanOrEqualTo algorithm
12c3c81 
- When the input > all stops it returned the second-last stop, should have been the last stop.
```
findStopLessThanOrEqualTo([0, 1, 2, 3, 4, 5, 6, 7, ], 8) // 6 but should have been 7
```

- When more than one stop had the same value it didn't always return the last stop
```
findStopLessThanOrEqualTo([0.5, 0.5], 0.5) // 0 but should have been 1
```
mourner
mourner previously requested changes Apr 9, 2019
View reviewed changes
Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

This looks great, thanks for the fix! Can you cover the new behavior with some tests?

src/style-spec/expression/stops.js Outdated Show resolved Hide resolved
src/style-spec/expression/stops.js Show resolved Hide resolved
@Mike96Angelo
Copy link
Contributor Author

I have made the changes and wrote some tests. let me know how the changes look.

@Mike96Angelo
Copy link
Contributor Author

@mourner have you been able to look at the changes I've made?

ryanhamley
ryanhamley approved these changes Apr 11, 2019
View reviewed changes
Copy link
Contributor

@ryanhamley ryanhamley left a comment

Choose a reason for hiding this comment

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

The changes look good to me. Thanks! 🎉

@ryanhamley ryanhamley dismissed mourner’s stale review April 11, 2019 20:57

Changes were fixed

@ryanhamley ryanhamley merged commit 0264f47 into mapbox:master Apr 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryanhamley ryanhamley ryanhamley approved these changes

@mourner mourner mourner left review comments

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
@Mike96Angelo @mourner @ryanhamley