fix swapped argument required vars in bitmaptools args helper#10253
Merged
Conversation
dhalbert
approved these changes
Apr 15, 2025
Collaborator
dhalbert
left a comment
There was a problem hiding this comment.
I agree with your conclusion -- not sure why it was the other way. I checked the other uses and the meaning you suggest remains consistent.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I believe the intention of this helper function is to have
if_required1be relvant forx1andy1and haveif_required2be relevant forx2andy2.That would mean that it is possible to control the required-ness of x1,y1 and x2,y2 independently.
As it is written now instead
if_required1is relevant forx1andx2andif_required2is relevant fory1andy2. If this functionality does match the intention then I would proposeif_requiredXandif_requiredYas more descriptive variable names.But I believe the x1,y1 and x2,y2 delineation was likely the original intention as it's plausible there could be a function that requires one coordinate point, but has the second one be optional. And seems less plausible to me that a function would have x1,x2 required and y1,y2 optional or vice versa for instance.
The only place where this helper function is called that the
if_requiredarguments actually differ is here: https://github.com/adafruit/circuitpython/blob/main/shared-bindings/bitmaptools/__init__.c#L722Using this statement which calls the affected function:
under
10.0.0-alpha.2results in this exception:using this PR branch instead successfully runs as the docstrings indicate it should if x1 and y1 are omitted.
I do also believe there are other issues with the arrayblit() arg validation and/or docstrings, but it's separate from this issue with the swapped values in the helper function here, it just so happens to make use of this.