-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Support list of list in draw.line
#3738
Comments
draw.line
draw.line
I can see that it could be useful. I've created PR #3739 to resolve this. |
@radarhere thanks! |
My initial thought is how about all the other places we have parameters that take a tuple (eg. A quick search on when to use a tuple or a list gives https://stackoverflow.com/a/1708538/724176:
And https://stackoverflow.com/a/1708538/724176:
That suggests tuples are better here, and not to use lists. Is there a compelling reason to also accept lists? |
@hugovk I agree with you on that However I think there are two compelling reasons to accept
|
@hugovk while you're in the mindset of reviewing things, are you interested in making a final call for this? I don't have strong feelings either way. |
Hmm, I also don't have strong feelings either way. It's adding a bunch of conversion code, which I doubt would have much of a performance hit, but means we should try and be consistent everywhere. Also, tuples and lists are for slightly different things (immutable/mutable, structure/order). Does that really matter? I think maybe it does: semantically you know a coordinate like |
@hugovk @radarhere Can we modify the PyPath_Flatten directly? And Modify the codes below: Lines 177 to 180 in 7585136
Lines 195 to 198 in 7585136
Lines 224 to 227 in 7585136
I think this way may
|
@hugovk @radarhere I have put a pr #3951. You can review and check that is there any problem with it? |
Thanks for your enthusiasm @gaoxinge, but it sounds like we're leaving it as is. |
Hi, I was kindly linked here after raising #6282. I appreciate this "what should we accept as parameters" is an old issue, and I am reluctant to re-open it, but I believe it deserves another look. I think the number of issues continuing to being raised around this topic demonstrate this hasn't been resolved yet. @hugovk wrote above (27 Mar 2019) about the differences between lists and tuples - particularly mutability, performance, and structure versus order. I understand and agree with the distinctions being made; they are important ones. Where Pillow is storing and processing coordinates, this makes a strong argument that they should be stored as immutable tuples. However, the right types for storage isn't the question being asked by these issues. Instead, the question being asked is "What types should be accepted via the Pillow API?" There is a standard answer to the question about what should be accepted via an API, and that answer underlies the design of the Python language. The answer is "Use duck-typing" - to give the most flexibility to the API client, you should accept any value whose type that looks and quacks like the type you need. When creating an new image, Pillow needs a size parameter. It doesn't need the size parameter to be an immutable, well-structured, performant 2-tuple of integers. It needs the size parameter to support the #3738 is about a client wanting to provide a coordinate as a list instead of a tuple. That is fine, and should be acceptable. #6282 is about (my) client wanting to provide a class instance that supports all the necessary operations as a coordinate instead of a tuple. That is fine, and should be acceptable. It is worth noting that different parts of the interface currently treat this differently.
I think the latter is the right approach†, and should be the direction that Pillow heads for all of the issues about what may be passed as a parameter. Any call to † (except Color3DLUT then converts the size parameter into a list (!!) for storage. It should be a tuple instead for the reasons provided by hugovk.) I know you made a decision about this in 2019 and it may be easier just to consider the topic closed, but I still invite you to reconsider. Thank you. |
A list of lists has now been enabled in #8800 |
Code below is running normally:
But if I change
into
the code will raise exception because the points are represented by list.
Is it appropriate that
draw.line
support the list of list as arguments?My pillow version is
5.4.1
.The text was updated successfully, but these errors were encountered: