-
-
Notifications
You must be signed in to change notification settings - Fork 98
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
Add Python-style list comprehensions to GDScript #2972
Comments
I don't know, to me this is way less readable. This doesn't read like a natural language sentence nor like a logical statement. Maybe there is a better example out there, but from this one — this is confusing way to express the intent with code. Also, to answer your question from the linked closed PR:
Code readability is one of the most important metrics when designing a programming language. Writing code takes way less time than reading it in one's career and day-to-day. |
@pycbouh it is read like "get element for every element in list where element satisfies condition" so I'd argue it is a lot readable |
|
"get x for x as each element where 'a' is in x" this is literally english |
@kahveciderin You add a bunch of words to make it literally English, words which are not present in the expression itself. You're used to it which is why it makes sense to you to add them. I'm explaining how it looks like to someone who'd see that code for the first time, before they learn to decipher it. I can agree that the whole line can be read as
which is fine. It's just a declarative way to express the same as
which more closely represents a block graph of the same algorithm. The benefit of the latter is that each logical part takes its own line and doesn't overload one with information in one swift blow. |
Consider this example: |
Well, IMO turning nested logic into a one-liner always worsens the readability. LOCs are not a resource you can run out of, so I see no reason to try and be as terse as possible. I have no idea what the code you've shown here does. |
For reference, GDScript already has a Python-style ternary operator ( I don't think the situation would differ much with list comprehensions. It's a lot of work to implement for a feature that may not be used much among the general Godot community. In general, I've found that advanced syntax features aren't used often (in contrast to language features, such as more powerful OOP/FP capabilities). Also, in Godot 4.0, Array has |
@Calinou You state that advanced syntax features aren't used that often. May I ask where you got that data from? Helping people on Discord? Was there a poll, that I am not aware of? Or what? I am not saying you are wrong. I am just curious. Regarding the list comprehension feature: IMO at least in theory the readability of list comprehension is objectively superior to that of an equivalent loop (ignoring deeply nested loop, because these are kinda a pathological case). The list comprehension makes it obvious immediately what is happening: We are making a list from something. If we look at a loop instead we need to parse it first before we can identify what is happening. In practice things look a bit different (as usual), because:
In regards to array filter/map. These only cover a very small subset of cases list comprehension can be used for. You could chain up these two function, but that is probably not great for performance and still doesn't cover all cases. Furthermore these function also only allow for building of lists from other lists. This is not a great replacement for list comprehension. |
It's from my experience of helping people out on various community platforms and reading other people's GDScript codebases. |
I see. Thanks. I am dubious of the former, because these platforms will have a larger percentage of beginners who naturally don't use advanced features much. The later is convincing though. I was going to argue that it is more a lack of knowledge than the advancedness of the feature that stops people from using it and that if we could convince GDQuest or KidsCanCode to make a video about (for example) the ternary operator we would see them in codebases all over the place in no time. But then I found a GDQuest video that covers the ternary operator. Seems my argument has been beaten. |
there has already been a pr implementing this back in 2018, so I don't really think it is that much work considering it only added 446 lines (https://github.com/godotengine/godot/pull/15222/files) |
GDScript has been rewritten from scratch since then. That PR can't be forward-ported to the current |
I have to agree with @pycbouh. While a lot of things python does is good, I really don't like list comprehension, especially when it becomes nested. numbers = [[1, 2, 3], [4, 5, 6], [7, 8, 9]]
print([n for sublist in numbers for n in sublist if n % 2 == 0])
# > [2, 4, 6, 8] Not very readable, and you save what, 3 lines? Not worth. |
I also think there's no need to implement new (alternative) syntax only because it saves some lines of code and this syntax is used in some language. First of all it should make the code more readable. |
I think the examples above are less readable to people not familar with list comprephension because they are all one-liners. It's unfair to judge the readability of one-liners. # this is valid GDScript
# should we ban for loops because this is not readable :)
var foo = []; for sublist in numbers: for n in sublist: if n % 2 == 0: foo.append(n); print(foo) In practice, complex Python list comprehensions are more likely to be written in multiple lines: # list comprehension in multiple lines
# reads: `foo` will be an array of `n`s, to get `n`, ... (the rest part reads the same as raw loops)
foo = [
n
for sublist in numbers
for n in sublist
if n % 2 == 0
]
# the equivalent raw loop:
foo = []
for sublist in numbers:
for n in sublist:
if n % 2 == 0:
foo.append(n) List comprehension is less verbose and saves lots of indentations I think. It's also useful if we are going to have immutable variables (#820). The first The reason why list comprehensions are widely used is the same as we use |
You are right, they are more readable over multiple lines. Honestly I had never seen it formatted like that. |
Indeed, when we discussed it a bit on Discord someone posted an example spread across multiple lines and it was pretty much as readable as a set of nested loops and conditionals. But I was under the impression that this proposal was based around saving lines, which this formatting doesn't achieve. As for indentations, they add clarity to the scopes where each variable is available, so they are actually useful in an indent-based language. I am also concerned that while it is possible to format comprehensions nicely, it is also possible not to, which would in turn allow users to code an incomprehensible mess. Stricter language rules that help reduce bad or smelly code are better IMO. PS. @timothyqiu, that side-by-side was hard to read on mobile, I was utterly confused by all the duplication 🙃 |
@timothyqiu good point on immutable variables, I haven't considered that while suggesting @pycbouh in theory, you can format any code badly, so it is really up to the programmer to format it properly. and #1200 could help with that.
It sometimes saves lines, sometimes it may not, but one thing is clear and it is that adding 2 nested loops inside one function means 3 indentation, which is harder to read, follow and even sometimes scroll left and right constantly. |
The brackets around list comprehension acts as the scope. Things always go deeper and deeper within it, and is impossible to get unindented logically. So I don't think indention matters in list comprehension. |
Putting aside the issue of formatting list comprehension is to an equivalent for-loop as a for-loop is to an equivalent while-loop. While Loop / For Loop:
For Loop / List Comprehension:
Same thing. The second option in each case expresses the intend more explicitly and reduces the amount of boilerplate code (and therefore the amount of code a reader has to comprehend). No one would argue that we don't need a for loop because we have while loops. IMO we are seeing resistance to list comprehension only, because it is less familiar to many people. |
I want to note list comprehension is quite limited compared to for sublist in numbers:
if sublist ... :#<-do some checks
... And without such checks the code may be slow. Also I believe many users would use one-liners (just because it saves lines) which would make the code less readable |
@me2beats in Python, you can access the index of the loop with list comprehensions. You can still use ternary in list comprehensions, I didn't understand what you meant by "You also can't do some checks in the outer loop".
That could be a problem with for loops as well, as @timothyqiu has demonstrated, does that mean we should remove for loops too? |
@zinnschlag If it's semantics you want to keep, the same can be achieved with a chain of map-reduce-filter. It would probably be even clearer as the operations are clearly labeled. You have also mentioned before that Python syntax for this is less than ideal. Do you have a better reference that you can give for this proposal? |
@pycbouh Reduce does not really comes into it, since reduce produces a single value and not a list. map-filter could be used. But this is not ideal. Even ignoring the potential performance issue of building two lists successively instead of just one, this solution would be much more verbose (either separate function or lambda), probably introducing more complexness and hard-to-read-ness than even the Python syntax. Also filter and map are only available for arrays. They do not allow you to create lists just from anything iterable. I seem to remember that the new GDScript does introduce iterators, but I could be wrong about that. The problem with the Python syntax is that there is no clear indicator that allows you to immediately identify list comprehension when you see it. [x for y in z if a]. It starts with a [, which is already overused and then follows with a variable. You actually have to parse three tokens to identify the list comprehension. It kinda makes sense because [] (outside of specifying an index/key) is already used to denote list literals, which are new lists (same as list comprehension). Its kinda consistent, but still not ideal on the readability front. I suspect we could achieve better readability if we indicate list comprehension via a new keyword (though I have no idea what word to use for that). It could be either of these:
|
maybe |
@zinnschlag Not sure about Python, but in JS reduce can be used with a collection as its default value and you can put items in it as you iterate. Useful for flattening nested structures, for example. |
@pycbouh I was talking about the GDScript reduce. |
Pretty much, yeah :). The proposal has been around for 6 years supposedly. If that's true, It took 5 years for someone to make a PR and then drop it two days later. It took another year for someone to make a new PR.
Two maintainers reviewed the most recent PR almost immediately. You can see their detailed comments and line-by-line feedback on the PR: godotengine/godot#81788. If you want to help the PR be ready as soon as possible after development on 4.3 starts, there's a lot you could do to help the maintainers and the author of the PR. For example, you could review the code yourself, help write unit tests, help write documentation, help bikeshed syntax and edge cases, help with QA by pulling the PR into your local branch and using it in your project. And again, all of these things will help you regardless of whether the PR is ultimately merged. If you help the author in these ways, you both can use a robust, well-documented, battle-tested feature. |
@nlupugla Why should we waste our time writing PRs when the maintainers are telling us not to and we have no idea if they are even considering the feature or not. If you think its due to a lack of motivation rather than people looking out for their own time I don't know what to tell you.
Yes they looked at the initial draft, but also said that they haven't come to a decision on it. My point is that even if the review feedback is addressed there's no chance they consider merging it in the near future
What would be the point of this? There is no indication that they are going to look at the PR when 4.3 starts, and there is no communication of whether they even want to consider the feature. A couple comments above they literally said to not work on PRs. Don't act like me and the other people discussing this feature are lazy and that's why it hasn't gained any traction. |
I don't mean to call anyone lazy, I'm sorry if it came off that way. In fact, I mean quite the opposite. Everyone is hard at work on their own projects!
Like I said, you can use the feature without the maintainers merging it into the official Godot build. There are multiple ways to do this, such as downloading the build artifacts from the PR or using git to pull the PR into your local repository and building the engine from source. I'd be happy to help you with either of those if you're unsure and I'm sure others (including the author, willing testers are almost always appreciated) would be as well. |
@nlupugla This is simply wrong. A PR was ready about a year later after the initial proposal, which eventually got closed, and can now not be merged because GDScript has changed so much in the meantime. |
Fair enough :) I didn't notice that PR when I searched for "comprehension". Nevertheless, the actions of the maintainers speak loudly here imo: they wouldn't have taken the time to review the most recent PR and leave such detailed comments if they were just going to dismiss it. There's clearly several people here who are very passionate about this proposal. I think that passion can be channeled productively in some of the ways I mentioned above, like testing the feature out in your own projects. Even if the maintainers decide not to merge the feature into the official Godot repo, you can still benefit from it in your personal projects. |
It has been resolved before though, check the comment that closed it. The associated PR was rejected in 2018 as the feature was deemed unwanted at the time. We allow new proposals for the feature (since proposals were introduced after that) but now it starts a new cycle. You cannot count the time from the original proposal because that was resolved, much less the time in between during which there has been no open proposal.
As mentioned, the PR got closed by a consensus of the maintainers about not wanting the feature. This PR was closed only a few days after being opened as well. There are older proposals here without resolution (and much less controversy) and don't have people constantly nagging the maintainers all the time to come to a conclusion ASAP, as if language design were the most trivial thing in the world and could be handled in 5 minutes. Such proposals also weren't rejected before. Just the fact we are still willing to discuss this shows how much we care for the community, as we could have closed this immediately after being opened on the grounds it has already been proposed and rejected before (though some people here seem too believe this would be better, not letting past decisions be reassessed). If we weren't willing to evaluate this it would have been closed long ago. But right now is also not a good time to do so, we have more pressing matters to focus on. We will discuss this with the maintainers after 4.2 is out and feature freeze is lifted. Let's stop with meta discussion here, it is not the place and won't be resolved like this. While the proper arguments for the actual proposal are now buried under this whole discussion that doesn't help reaching a resolution. |
This comment was marked as abuse.
This comment was marked as abuse.
There's a lot of thumbs down in this proposal (over 25% which is way higher than anything else). It is one of the most downvoted proposals. The controversy is undeniable. |
Expanding on the concerns raised by @nukeop, it is essential to highlight the issue of disrespectful treatment within the ongoing discussions. Throughout the dialogue, it appears that our genuine inquiries have been portrayed as persistent pestering, as exemplified by the rather unfortunate choice of words, such as "nagging," which was directly used by @vnen in his comment about an hour ago. In reality, our primary objective is not to pester, but rather to foster an environment of transparency, which is crucial for any open-source community. What has been particularly disconcerting (at least for me) in this regard is the limit that was put on the discussion part of my PR, along with the labeling of comments that the development team might find unfavorable as spam or off-topic. This approach seems somewhat immature and is not in line with the expectations one might have from a development team of this scale and importance in the open-source community. At this point, I feel like our voices aren't being heard, and no matter what valid concerns we raise it would be swept under the rug because we're being persistent, and that is weirdly viewed by the team as trying to force the proposal to be accepted, when in reality that's not what we are trying to do at all. It's almost as if the entire team is trying to find excuses not to see our valid concerns, and nitpicking specific words/sentences/comments from the discussion to blame us for wanting transparency and communication. Nobody is forcing anyone to make a specific decision, for or against this proposal yet somehow we're always being blamed as if we are trying to cause a fight here. Sorry, but nobody here can change my mind about the absolute metric ton of disrespect that we are receiving here. Some notable examples are:
There's a gut feeling in me that the discussion won't resume even after 4.2 and then "after 4.3" would be the quoted time for the discussion. But hey, hopefully I am wrong. |
I just want to preface this by saying I truly appreciate the work of all the maintainers and I understand it is a tough and thankless job. I was not trying to say that I think you guys are doing a bad job or don't care about the community because that is not what I think at all. I don't support the mean comments and harassment from others as well. I feel like you guys see my criticism as me being upset that this specific feature hasn't been accepted when that is not the case. I have honestly given up on this feature being accepted a while ago, and would much rather see it be rejected than be in the state it is now. It is frustrating to see the years of no progress on this and no clear communication or instructions on what to do about that. People in this thread are described as nagging when we are just trying to make forward progress. Can you please just give us clearer communication on what we can do to progress this or when exactly it will be discussed if we are just waiting for that? Every release we have just been told this will be deferred to the next release.
How can you say things like this as if they aren't a bad thing? I don't hope for any proposals to be in limbo for years and years. I think taking steps to be more clear with the proposal process and potentially speed it up could help waste less of everyone's time |
@JohnnyUrosevic The reason why contributors are frustrated and call this behavior nagging is because every time you ask for clear guidelines to help move this forward you are given one: There is nothing you can do to help at this point, it needs to be discussed by a very busy GDScript team, a team which has other priorities right now, and yes, those priorities span YEARS of development. It was also mostly a one-man effort for a significant chunk of this proposal being alive in one form or another. So it will take time. Time that you can not speed up. You can only wait. And we will give you any information we have, any resolution that is reached, when we have it. And that's clear and transparent answer that has been given time and time again. And yet, several of you keep saying that we are not giving any answer. So what are we to take from your perceived dismissal of this answer, if not the idea that you want us to resolve this now, immediately, and no answer that contains words "You have to wait for news" is going to be accepted?
It would be a bad thing if we didn't go through a hundred merged PRs a week, and didn't just get 1300 changes into 4.2 in the last 3 months. You are frustrated that one feature is not merged for years, while the team of hundreds of contributors have shipped thousands of improvements during that time. Please, have some perspective. |
Why do you keep characterizing my criticism and my frustrations of trying to contribute to your project as me feeling entitled to have this feature merged? How many times do I have to say I'd rather you just close the proposal than waste more of my time. I know you have said to "wait" over and over again, but I don't think you are being as clear as you claim you are. Again you said wait until 4.0, 4.0 then released and we asked whats going on with this proposal, as you instructed us too and then you act like we are nagging you and are being entitled when we did the thing you told us to do. How many more releases are we going to have to wait for with you pushing it back again? What are we waiting for? Telling us to wait indefinitely is not a clear or helpful answer. It just sucks seeing a project I was excited to contribute to and being dismissed and called a child for voicing minor concerns. I'm sure others feel the same way |
|
Okay is there any roadmap of when they plan to discuss things? Do they just pick features to discuss randomly? Is there some sort of prioritization system? Is there things they are currently discussing that are taking up lots of time that we can help with? Is there some way to help with the decision process? I am trying to help and you constantly dismissing me is not constructive or appreciated. I get that you are frustrated at me but I think you have some warped perception of what my priorities are |
This comment was marked as abuse.
This comment was marked as abuse.
I am not dismissing you. I'm saying that for this proposal you've done enough, everyone has done enough already. Now the ball is in someone else's court. Can you accept that someone else needs to find time and work on this now, and you can't help it? |
You are dismissing me and haven't addressed a single criticism I've raised. You are just going to continue being rude to me and ignoring what I'm directly asking you so I am done here. Good luck everyone |
Hey all, It seems like this proposal, in the state it's now, it's not really constructive. I can see why there's frustrations on both sides but continuing to throw arguments right now won't bring this anywhere. To address transparency: work is done and coordinated both on github and the developer chat. The perceived lack of transparency on all of this is most likely due to not engaging with the chat platform. While this may not be immediately clear, Godot has already taken big steps in improving the response times to proposals and PRs over the years. Please be patient. If you want to engage with development and understand what's going on, you can join the developers chat. You can help review PRs. You can PR fixes to other bugs. However, please listen to maintainers if they ask to temporarily drop a subject. When we give timelines they're always tentative. Most of the work on this project is done by volunteers, which means that having a precise timeline is really really hard as people's availability is highly variable and hard to plan around. |
@QbieShay Thank you for actually responding to what I asked. Transparency is all I ever asked for |
This comment was marked as abuse.
This comment was marked as abuse.
During the end of the RC period of Godot 4.2, the GDScript team decided that it was a good time to discuss about #2972 and godotengine/godot#81788. Notwithstanding some issues that members have about list comprehensions (notably about readability), and even if, for the sake of the argument, we conclude that they aren't bad per se and that they aren't not needed at all, it doesn't mean that list comprehensions shall be included in the language. As a team, we don't see how list comprehensions are needed for GDScript. For the few features that feature brings, which are already covered by We know that there's a lot of supporters of having list comprehensions inside the language, but there's a lot of people that are skeptical about the idea too. The fact that most of scripting languages (aside Python) don't support that feature is an indication too. So we concluded, after a lengthily meeting, to close the list comprehension proposal and the related PR for the stated points and because the idea was already rejected before the new proposal system. I've taken the time to reread the entire thread to be sure that I've not missed something critical. Unfortunately, it seems that there's no consensus. Even if this thread is very vocal about having list comprehensions, the reactions to the proposal are mixed at best. And it's an issue that will not be solved with "arguments", unfortunately. The points that were brought forward were quite good and explain well how list comprehensions can be readable. But it's a design choice to not have list comprehensions. The GDScript team doesn't believe, right now, in investing in list comprehensions. The arguments haven't convinced us. We don't have the habit to close proposals. Usually, we let them open to see if they're gonna get traction with time, even if we disagree as a team with them at first. But we did tell a lot of people here to wait for the release of 4.0, 4.1 and 4.2. That for reason, we'll close this one. I'm sincerely sorry for the time you spent waiting for a response. It's something that we shall work as a team, in the future, to prevent similar occasions. As @QbieShay said, I highly encourage you to visit the developer chat, as GitHub is only a part of the development process. The GDScript team is (publically) meeting each week (see the shared calendar). See you there! |
This comment was marked as abuse.
This comment was marked as abuse.
@nukeop This decision was made by the GDScript team, we do not appreciate you shooting the messenger like this. It's not the first time you breach our Code of Conduct, disrupt work, and disrespect contributors, so the Code of Conduct team decided to ban you from interacting on the Godot GitHub organization. |
Describe the project you are working on
Creating constant dictionaries, filtering and mapping arrays, conditional logic in array creation.
Describe the problem or limitation you are having in your project
this could be written in a single line like so:
Describe the feature / enhancement and how it helps to overcome the problem or limitation
As discussed in this pull request and this issue, it isn't harder for people to read list comprehensions, even easier to read than multiple nested for loops.
Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams
If this enhancement will not be used often, can it be worked around with a few lines of script?
It will be used often
Is there a reason why this should be core and not an add-on in the asset library?
This changes gdscript.
The text was updated successfully, but these errors were encountered: