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

Bug fix: image() had wrong args parsing logic #382

Merged
merged 4 commits into from
Aug 25, 2022
Merged

Bug fix: image() had wrong args parsing logic #382

merged 4 commits into from
Aug 25, 2022

Conversation

tushar5526
Copy link
Member

Refactored image.py to use constants instead of strings

@tushar5526
Copy link
Member Author

tushar5526 commented Aug 24, 2022

Hi @ziyaointl, the mixed-use of tuples and standard variables in function calls is causing trouble here.
So for a call like image(img, (x,y), (w, h)) would lead to len(args) being 3 and according to the old logic of args parsing this would lead to the following parsing of args. img = args[0], and location = args[1:] which is wrong.

It seems there is a random mix of using tuples and standard variables. I am thinking of modifying this image API only to use standard variables instead of tuples. Writing a complicated parsing logic that supports tuples, standard variables or a random mix of them does not seem productive.

Similarly, we should drop support for tuples in other APIs later.

@ziyaointl
Copy link
Member

Yes, I think decreasing the complexity of the interface is a worthy goal to pursue, and I agree that removing the tuple interface is a good way of doing that. The tuple interface was a good attempt to make p5py more pythonic, but not being compatible with other processing languages at the API level is inconvenient for most existing processing users, and like you said, the complexity of maintaining two APIs is showing its downside of being harder to maintain.

This said, we should be very clear in our communication with the users about this change, since this does break previous p5py programs that use the tuple API. We should make sure that any API deletion occurs alongside a major version bump, and we should highlight the deletion in very obvious ways in the changelog.

@ziyaointl ziyaointl merged commit 33da3d7 into master Aug 25, 2022
@ziyaointl ziyaointl deleted the image branch August 25, 2022 02:00
@tushar5526
Copy link
Member Author

Related to #379

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

Successfully merging this pull request may close these issues.

2 participants