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

Fix validity check for IJK+ coordinates #271

Merged
merged 2 commits into from
Aug 12, 2019
Merged

Fix validity check for IJK+ coordinates #271

merged 2 commits into from
Aug 12, 2019

Conversation

saprykin
Copy link
Contributor

@saprykin saprykin commented Aug 8, 2019

No description provided.

@CLAassistant
Copy link

CLAassistant commented Aug 8, 2019 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@isaacbrodsky
Copy link
Collaborator

Thank you for catching this! Would you mind adding a test case to demonstrate this is fixed?

@coveralls
Copy link

coveralls commented Aug 8, 2019 •
edited
Loading

Coverage Status

Coverage remained the same at 100.0% when pulling 607640e on saprykin:master into 576096c on uber:master.

@saprykin
Copy link
Contributor Author

saprykin commented Aug 9, 2019

I added additional test cases when j and k components of IJK coordinates are out of range. With previous version new tests are not failing though they must. Hopefully I added it to right place.

isaacbrodsky
isaacbrodsky approved these changes Aug 9, 2019
View reviewed changes
Copy link
Collaborator

@isaacbrodsky isaacbrodsky left a comment

Choose a reason for hiding this comment

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

I applied this branch locally and confirmed that line 293 does get covered now.

@nrabinowitz nrabinowitz merged commit b6210f7 into uber:master Aug 12, 2019
isaacbrodsky pushed a commit to isaacbrodsky/h3 that referenced this pull request Aug 12, 2019
Version 3.5.1, add CHANGELOG for uber#271, update to point to uber#267 …
2de2ba4 
…(the PR instead of issue)
@isaacbrodsky isaacbrodsky mentioned this pull request Aug 12, 2019
Merged
mrdvt92 pushed a commit to mrdvt92/h3 that referenced this pull request Jun 19, 2022
Merge pull request uber#271 from saprykin/master
670b62f 
Fix validity check for IJK+ coordinates
mrdvt92 pushed a commit to mrdvt92/h3 that referenced this pull request Jun 19, 2022
Version 3.5.1, add CHANGELOG for uber#271, update to point to uber#267 …
01ff652 
…(the PR instead of issue)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@isaacbrodsky isaacbrodsky isaacbrodsky approved these changes

@nrabinowitz nrabinowitz nrabinowitz 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.
5 participants
@saprykin @CLAassistant @isaacbrodsky @coveralls @nrabinowitz