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

Update the comparison implementation in voronoi.py (fixes #18219) #7554

Merged
merged 3 commits into from
Aug 7, 2018

Conversation

havatv
Copy link
Contributor

@havatv havatv commented Aug 7, 2018

Description

New (after second commit): The cmp function introduced with the Python 2 -> 3 upgrade contained a greater than (>), but the gt function was missing in Site and Halfedge. I have changed cmp to only use less than (<), and have modified the lt functions to return False in the last else.
Old: When "translating" the cmp function of Site and Halfedge to Python 3, the gt function was forgotten. It has been added.

Checklist

Reviewing is a process done by project maintainers, mostly on a volunteer basis. We try to keep the overhead as small as possible and appreciate if you help us to do so by completing the following items. Feel free to ask in a comment if you have troubles with any of them.

  • Commit messages are descriptive and explain the rationale for changes
  • Commits which fix bugs include fixes #11111 in the commit message next to the description
  • Commits which add new features are tagged with [FEATURE] in the commit message
  • Commits which change the UI or existing user workflows are tagged with [needs-docs] in the commit message and contain sufficient information in the commit message to be documented
  • I have read the QGIS Coding Standards and this PR complies with them
  • This PR passes all existing unit tests (test results will be reported by travis-ci after opening this PR)
  • New unit tests have been added for core changes
  • I have run the scripts/prepare-commit.sh script before each commit

When "translating" the cmp function of Site and Halfedge to Python 3, the __lt__ function was forgotten.  It has been added.
@nyalldawson
Copy link
Collaborator

Thanks for your hard work here!

I'd love to get a unit test in place protecting this situation. Are you comfortable adding one, or would you like some guidance?

@havatv
Copy link
Contributor Author

havatv commented Aug 7, 2018

I have no experience with unit tests, but I could provide a dataset that covers more cases than the existing one. Several datasets would be needed to cover more cases.
Is it enough to add input and result datasets and updating qgis_algorithm_tests.yaml?
And sorry for changing so much in the latest commit - I found out that the gt function could be avoided by changing the cmp function to only use < (less than). The combined changes of these commits have now become very small.

@havatv
Copy link
Contributor Author

havatv commented Aug 7, 2018

I tried to add a test in pull request #7557.

@nyalldawson nyalldawson added this to the 3.4 milestone Aug 7, 2018
@nyalldawson nyalldawson merged commit ca00174 into qgis:master Aug 7, 2018
@nyalldawson
Copy link
Collaborator

Thank you!

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

Successfully merging this pull request may close these issues.

2 participants