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

Make dict comprehension on one line #243

Closed
wants to merge 1 commit into from

Conversation

jasongrout
Copy link
Member

I had to stare at this for a long time, while my brain was parsing the statement one line at a time, trying to figure out what in the world it was doing. I then realized it was a dict comprehension. Personally, I think this comprehension makes a lot more sense when scanning through the code when it is on one line.

I realize this is a stylistic issue, so if I'm outvoted, fine. This comprehension took me enough time to understand (or even syntactically recognize as a dict comprehension) that I thought it was worth a PR.

@ssanderson
Copy link
Contributor

As the person who wrote this expression, I'm -1 on this change.

I prefer multi-line comprehensions over one giant line because I think it clearly differentiates the three operations that are happening in the comprehension:

{
     <expression>
     <iteration>
     <filter>
}

Maybe I'm just used to reading my own style, but I find this separation much clearer than having to parse the logical sections in a long line by hand.

@minrk
Copy link
Member

minrk commented Jul 30, 2015

I don't have a strong feeling. I don't personally find a one-line comprehension easier to follow than a multi-line one, if anything it's slightly the other way around. I'm not especially fond of filtered comprehensions in general.

@ssanderson
Copy link
Contributor

The multiple bindings of the name key in the expressions here make this unusually hard to parse I think. A totally sane alternative is also to not use a comprehension at all:

errors = {}
for key in maybe_none_keys:
    if model[key] is not None:
        errors[key] = model[key]

This feels verbose to me, but it's almost certainly the most straightforward spelling.

@minrk
Copy link
Member

minrk commented Jul 30, 2015

I'd be AOK with removing the comprehension if that's helpful to people. I don't think reshaping the comprehension improves it, though.

@jasongrout
Copy link
Member Author

I think why I struggled to understand it were:

  1. not used to dict comprehensions
  2. having for and if on separate lines helped me parse them as for and if statements, which threw my brain for a loop when it just didn't make sense
  3. The list comprehension a few lines above is almost the same thing, but on a single line

In general, I think comprehensions are more efficient than explicit loops, so I prefer them, so I'll close this as a style issue.

@jasongrout jasongrout closed this Jul 30, 2015
@jasongrout
Copy link
Member Author

P.S. It wasn't just me. I showed it to @SylvainCorlay as well, and he stared at it blankly for a few seconds before I explained what it was.

@minrk
Copy link
Member

minrk commented Jul 30, 2015

@jasongrout if you wanted to keep this PR and turn it into the for-loop, that would be fine. I don't think this is a case where any efficiency benefits of a comprehension will be noticeable, since it's also talking to a filesystem or external data store.

@minrk minrk modified the milestone: no action Dec 1, 2015
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants