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

Consistently validate location on VectorShape #5327

Merged
merged 4 commits into from
Sep 11, 2021
Merged

Consistently validate location on VectorShape #5327

merged 4 commits into from
Sep 11, 2021

Conversation

lesamouraipourpre
Copy link

Closes #5325

The x, y or location properties now use the same bounds validation whether set via the constructor or setters.
Tested with the code in #5325 which now raises Errors consistently when out of range.

@WarriorOfWire can you check if these changes are consistent with the rewrite and pass any test code you have?

@lesamouraipourpre
Improve the bounds checking on the location (the x, y co-ordinates) of a
1f44691 
VectorShape object so that it is consistent no matter where it is set from:
 * the constructor
 * the x and y setters
 * the location setter
@lesamouraipourpre
Remove unused varg part of Error calls.
c6f2dae
@microdev1 microdev1 added this to the 7.0.0 milestone Sep 10, 2021
kvc0
kvc0 suggested changes Sep 10, 2021
View reviewed changes
Copy link

@kvc0 kvc0 left a comment

Choose a reason for hiding this comment

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

Validation consistency changes look good! I am concerned about breaking the correct semantic of the module set_location family functions.

shared-bindings/vectorio/VectorShape.c Outdated Show resolved Hide resolved
@lesamouraipourpre lesamouraipourpre marked this pull request as draft September 10, 2021 15:29
@microdev1 microdev1 removed this from the 7.0.0 milestone Sep 10, 2021
@lesamouraipourpre
Copy link
Author

@WarriorOfWire I've made those changes, keeping the only changes at the shared-bindings level to the usage of int32_t instead of int16_t. All the other changes are in shared-module.

Do the vectorio_vector_shape_validate_x_bounds and vectorio_vector_shape_validate_y_bounds function names make sense within CircuitPython best practices?
Also, is there an easy way to pass a QSTR into a function? As these two functions could be merged if I can pass in the variable name for the error message, eg. MP_QSTR_x or MP_QSTR_y

@lesamouraipourpre lesamouraipourpre marked this pull request as ready for review September 11, 2021 16:14
microdev1
microdev1 approved these changes Sep 11, 2021
View reviewed changes
Copy link
Collaborator

@microdev1 microdev1 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the PR @lesamouraipourpre and @WarriorOfWire for the review.

@microdev1
Copy link
Collaborator

microdev1 commented Sep 11, 2021 •
edited
Loading

Will wait for thoughts from @WarriorOfWire before merging.

kvc0
kvc0 approved these changes Sep 11, 2021
View reviewed changes
Copy link

@kvc0 kvc0 left a comment

Choose a reason for hiding this comment

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

This is good, thanks!

@microdev1 microdev1 merged commit cf5c32b into adafruit:main Sep 11, 2021
@lesamouraipourpre lesamouraipourpre deleted the vectorio branch September 11, 2021 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@microdev1 microdev1 microdev1 approved these changes

@kvc0 kvc0 kvc0 approved these changes

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

Successfully merging this pull request may close these issues.

Vectorio doesn't consistently validate location
3 participants
@lesamouraipourpre @microdev1 @kvc0