Conditional dict declarations
Hi list I have a proposal for a minor (improvement?) to dict declarations. I find a common requirement I have is to include one or more entries in a dict only if some condition is met. Currently, the usual way of doing that is to add one or more if statements. Simple (but hopefully relatable-to-real-world-code) example. Here we're constructing a query for a database based on the arguments passed in. In some cases we don't want to include some fields in the search at all depending on what the values passed in are. ### def construct_query(user_id, max_results=10000, active=None, deleted=None): query = { 'user_id': user_id } if max_results > 0: query['max_results'] = max_results if active is not None: query['active'] = active if deleted is not None: query['deleted'] = deleted return query ### This is OK-ish, but seems unnecessarily verbose, and it gets worse if you have more conditional entries. In addition, you're separating the dict setup code into two or more places so it is slower to grasp mentally. This is a fairly common (tedious) pattern in my code, but I feel it could be avoided. In my view it would be better if it was possible to conditionally add the entry in the dict declaration. My proposal is that there should be a special value defined that results in a dict entry not being added to the dict if either the key or the value equals this value. There may be several ways to achieve this, but here's one way it could work. ### def construct_query(user_id, max_results=10000, active=None, deleted=False): return { 'user_id': user_id, 'max_results': max_results or dict.skip, 'active': active if active is not None else dict.skip, 'deleted': deleted if deleted is not None else dict.skip } ### ..."skip" being a unique object defined in and accessible from dict (there may be better places to make it available). The dict insertion code then simply ignores those entries when creating the dict. I think this would be relatively easy to achieve. No significant modification to the language, just a minor modification to the dict code (as far as I know).
On Thu, 5 Sep 2019 at 11:19, <rls@hwyl.org> wrote:
This is pretty easy to write as a helper function:
Given this, I'm not sure it's a sufficiently useful improvement to be worth adding to the builtin dictionary (although thanks for the suggestion, I wouldn't have ever thought about writing a helper to avoid the boilerplate without this posting to prompt me!) Paul
I was thinking more in this direction: d = dict() d[action in not None and 'action'] = action d[minval > 0 and 'minval'] = minval ... del d[False] There are few things I do not particularly like about this (assigning to dict, even when value is discarded later, or necessity of the del d[False]), but I guess it is as expressive as it could be and I could imagine using it in my app easily. If needed one can even define another key instead of False.
Oooookie, looks like my code got munched in the web view -- how do I make it not do that?
How is this different to the discussion https://mail.python.org/archives/list/python-ideas@python.org/thread/MFSL3U6... ? On Thu, Sep 5, 2019 at 9:24 AM <brian.skinn@gmail.com> wrote:
-- Sebastian Kreft
On Sep 5, 2019, at 07:46, Sebastian Kreft <skreft@gmail.com> wrote:
How is this different to the discussion https://mail.python.org/archives/list/python-ideas@python.org/thread/MFSL3U6... ?
Well, that discussion started with an unworkable proposal, and generated a variety of different alternatives, some of which were workable, but all of which were buried deep in the discussion of the unworkable idea. Polishing up one of those alternatives and extracting it out into its own new thread doesn’t seem like a bad thing to do. A link back to the old thread might have been nice, but now you’ve fixed that. :)
Also, if one have in mind that dict addition with the `+` operator had been recelently approved, as of Python 3.9 it will be ok to write: ``` return ({ 'user_id': user_id,} + ({'max_results': max_results} if max_results else {}) + ({'active': active} if active is not None else {}) + ({'deleted': deleted} if deleted is not None else {}) ) And actually, on a second though, this is already valid as of current Python - and is potentially more readable than a magic "dict.skip" singleton. ``` return { 'user_id': user_id, **({'max_results': max_results} if max_results else {}), **({'active': active} if active is not None else {}), **({'deleted': deleted} if deleted is not None else {}) } ``` On Thu, 5 Sep 2019 at 10:22, <brian.skinn@gmail.com> wrote:
On Thu, Sep 5, 2019 at 8:37 PM Joao S. O. Bueno <jsbueno@python.org.br> wrote:
Also, if one have in mind that dict addition with the `+` operator had been recelently approved
Where did you hear this? I am not aware that this was even made into a PEP let alone that such a PEP was approved. -- --Guido van Rossum (python.org/~guido) *Pronouns: he/him/his **(why is my pronoun here?)* <http://feministing.com/2015/02/03/how-using-they-as-a-singular-pronoun-can-c...>
On Fri, 6 Sep 2019 at 01:54, Guido van Rossum <guido@python.org> wrote:
Well - the idea showed up here, and got an immediate "that is nice, let's do it" from you, followed by approval from a lot more people, and very little controverse. So, it is pretty much "approved", even if not yet detailed in an approved PEP. And 3.9 still sounds quite realistic for that specific feature, unless no one actually writes the PEP. But my bad, I thought the full approval process (PEP included) for that had been carried on already. Nonetheless, the mapping expansion syntax do allow for the OP's needs, and is even a bit shorter than the version using "+".
On Sat, Sep 07, 2019 at 12:35:09AM -0300, Joao S. O. Bueno wrote:
As the PEP author, I don't consider it even close to approved. I'm still slowly working my way through the next draft of the PEP. There are lots of objections and unresolved questions: - is this feature necessary? - if so, should it be an operator, a dict method or a function? - if an operator, should it be spelled ``+`` or ``|`` or something else? - what precise behaviour should it have? At the time I wrote the first draft, there was no consensus that the feature was necessary, but most people who thought it would be useful agreed that ``+`` was the right operator. Since then, further discussion seemed to shift closer to consensus that the feature might be useful, but away from using ``+``. -- Steven
On Sat, 7 Sep 2019 at 01:24, Steven D'Aprano <steve@pearwood.info> wrote:
Thank you for the feedback. I will follow the PEP evolution more close now. And indeed, as we can see, even in a case like this in the thread, where the "+" could have been useful (1) the OP did not think of using it at all, coming up with a different approach, and (2) the exiting star-map expansion can do what "+" would do in this case. These would point to that maybe it is not "necessary" at all. Although I still think it would be a useful feature, and, on that, maybe keeping the consistence with the "|" already in use by 'set' would be better.
On Sep 4, 2019, at 15:25, rls@hwyl.org wrote:
..."skip" being a unique object defined in and accessible from dict (there may be better places to make it available). The dict insertion code then simply ignores those entries when creating the dict.
I’m a bit confused about exactly when this does and doesn’t come into play. You say here “the dict insertion code” does it. Would they mean d[key]=d.skip doesn’t insert anything? If so, then how could I put skip in a dict at all? If there’s already a d[key] does that get left alone, or deleted? Wouldn’t this even affect dict inserts that happen behind the scenes, like any time you tried to assign dict.skip to a global variable or a class or instance attribute, or pass it as a keyword argument? If an implementation doesn’t use dict.insert to build dict literals, does that mean it doesn’t work in dict literals on that implementation, even though that was the whole motivation for the feature? Maybe instead of starting with how it could be implemented, start with where you ideally do and don’t want it to take effect, and then we can try to work out how it could be implemented to do that, and how to specify it to require such an implementation. * Obviously you want it in non-comprehension dict displays, because that’s your example. * What about comprehensions? * Constructor calls with the keyword-arg API like dict(a=1, b=dict.skip, **d2}? * Constructor calls from iterables of pairs, like dict((1, 2), (3, dict.skip))? * Subscription assignments? * Calls to insert? * Calls to update? * Capturing ** keyword args? * Assignments to globals, attributes, etc. when those use a dict under the covers (as they usually do!? * Assignments to such things when they don’t use a dict (as with a slots class instance)? Also, what should happen with this case: flag0, flag1 = True, False d = {'k': 0 if flag0 else dict.skip, 'k': 1 if flag1 else dict.skip} Do we keep the last value for 'k' as usual, and because that’s dict.skip, therefore d['k'] ends up as a KeyError? Or do we keep the last non-skip value and therefore d['k'] ends up as 0? What about this: class D(dict): skip = 0 d = D() d.skip = 1 d[0] = 0 d[1] = 1 d[dict.skip] = dict.skip Do we skip the one that uses d.skip, the one that uses type(d).skip (like a special method lookup), or the one that uses dict.skip?
On Thu, 5 Sep 2019 at 16:51, Andrew Barnert <abarnert@yahoo.com> wrote:
You say here “the dict insertion code” does it. Would they mean d[key]=d.skip doesn’t insert anything?
Yes. If so, then how could I put skip in a dict at all? The intention is that you couldn't. This could raise some implementation issues!
If there’s already a d[key] does that get left alone, or deleted?
Left alone.
This was what I was attempting to do. I wasn't suggesting the way I wrote was the way it should be implemented, just a sketch of how it might look.
Yes.
* Constructor calls with the keyword-arg API like dict(a=1, b=dict.skip, **d2}?
I'd expect the b value to not be assigned.
* Constructor calls from iterables of pairs, like dict((1, 2), (3, dict.skip))?
I'd expect the 3 key to be unassigned (assuming an extra pair of brackets).
* Subscription assignments?
I'm sorry, I'm not sure what you're referring to here. Do mean as in x[dict.skip] = y or x[y] = dict.skip? If so, I'd expect both operations to be, well, skipped.
* Calls to insert? * Calls to update?
Yes. Inserts to remain unassigned, updates to keep the original value. The idea is to skip the operation.
* Capturing ** keyword args?
Yes.
* Assignments to globals, attributes, etc. when those use a dict under the covers (as they usually do!?
I'm not sure... setting an attribute/global to "dict.skip" might not have any effect. I don't think the user could complain about surprise for doing something like this.
I'd expect the assignments to happen in order, so d['k'] would be set to 0 and then the second assignment to be ignored.
This seems like a fairly pathological case, and I'm not sure it's really in the spirit of your request to focus on the use rather than the implementation! But I'd expect the key value of dict.skip to cause the entry to be skipped. Thanks for coming up with so many angles on this; it's interesting exercise to consider how many things could be affected by this. Writing a ConditionalDict type in a similar spirit would be a lot simpler, though less convenient to use. I will experiment. Thanks Rich
On 04/09/2019 23:25:32, rls@hwyl.org wrote:
if max_results > 0 : query['max_results'] = max_results if active is not None : query['active'] = active if deleted is not None: query['deleted'] = deleted If this comes out ragged in your browser, the colons and equal signs are meant to be vertically aligned. Rob Cliffe
On Sat, Sep 7, 2019 at 11:27 PM Rob Cliffe via Python-ideas <python-ideas@python.org> wrote:
I'm not bothered by putting an if and a simple statement together, but I am definitely bothered by the need to vertically align them, and especially by the triple repeated name. Not a fan of this style - looks like a bug magnet and maintenance burden. ChrisA
On 07/09/2019 18:59:49, Chris Angelico wrote:
if max_results > 0: query['max_results'] = max_results if active is not None: query['active'] = active if deleted is not None: query['deleted'] = deleted is perfectly reasonable and has the same triple repeated name. Would you rewrite it differently, and if so, how? I'm just saying that aligning similar elements and bringing them together, without intervening lines, makes it easier to see the code structure and spot typos. Not to mention using less vertical screen space. I don't understand why you think it is a bug magnet and maintenance burden. I think it is *easier* to maintain: in which of the following is it easier to spot the mistake? # Version 1: if max_results > 0 : querydata['max_results'] = max_results if active is not None : querydata['active'] = active if deleted is not None: quervdata['deleted'] = deleted # Version 2: if max_results > 0: querydata['max_results'] = max_results if active is not None: querydata['active'] = active if deleted is not None: quervdata['deleted'] = deleted Data point: I have used this style repeatedly in my code (only where appropriate, of course) and find it helpful. Rob Cliffe
On Mon, Sep 9, 2019 at 4:13 AM Rob Cliffe via Python-ideas <python-ideas@python.org> wrote:
Now add a fourth one, for result_count. What does your diff look like? Do you have to edit every other line just to realign it? That's the maintenance burden - fiddling around with formatting. Honestly, I couldn't see the mistake in _either_ version until I looked pretty closely, upon which it was equally visible in both. If I were using this in a full app, it would be something a linter could easily spot, again regardless of the alignment. You're right that the triple repeated name is not caused by your style. However, your suggestion also doesn't solve it. What I'd probably want to do, here, is build the dictionary with everything, and then have a separate pass that removes anything that's None (or maybe "any of these keys, if the value is None"), and then just special-case the requirement for max_results to be positive. But I'd be looking to make a broader change somewhere, taking into account a lot more code, rather than doing this whole "take local names and plonk them into a dict" thing at all. ChrisA
On Sep 8, 2019, at 11:34, Chris Angelico <rosuav@gmail.com> wrote:
If you really want to avoid repeating names, the only plausible option seems to be putting them in a namespace that you can then filter and dictify, instead of storing them as a bunch of separate locals. Well, I suppose you could use locals itself as that namespace: query_data = {k: v for (k, v) in locals().items() if k in {'max_results', 'active', 'deleted'} and v} … but that’s pretty horrible. And I suppose there is something Python could do to alleviate the repetitions. The stalled proposal for magic keyword arguments would let you write something like this: query_data = dict(max_results=, active=, deleted=) … which you could then filter. But I don’t think most people liked that proposal, and I don’t think this is a good enough use case to revive it.
On 08/09/2019 19:34:33, Chris Angelico wrote: they look too similar: if max_results > 0 : querydata['max_results'] = max_results if active is not None : querydata['active'] = active if deleted is not None: querodata['deleted'] = deleted Can you honestly say it's not easier to spot the inconsistency when elements that should be identical are adjacent? (BTW I changed the identifier from query to querydata because my spell-checker, and maybe yours, highlights the mistake instantly in both versions.) Rob Cliffe
On Sat, Sep 07, 2019 at 08:22:58PM +0100, Rob Cliffe via Python-ideas wrote:
Obviously the mistake is that max_results is supposed to be greater than OR EQUAL to zero. *wink* But seriously, most coding errors aren't simple spelling errors. If they are, chances are that you will immediately get a NameError, or (as you point out yourself) your spell checker will spot it. The insidious errors are logic errors and this does not help with those unless your lines are almost identical: if seq[0] == 1: query[0] == 0 if seq[1] == 1: query[1] == 1 if seq[2] == 1: query[2] == 2 in which case you should probably use a for loop. If this style works for you, I'm glad for you and don't wish to persuade you differently. But for me, I am not comfortable with this style and would not use it because I have seen far too many people (including myself) waste too much time farting about fixing formatting and alignment issues instead of doing productive work. # Oh no now I have to re-align ALL THE THINGS if max_results > 0 : querydata['max_results'] = max_results if active is not None : querydata['active'] = active if deleted is not None: querydata['deleted'] = deleted if items and items[0] >= items[-1]: querydata['items'] = items if max_results > 0 : querydata['max_results'] = max_results if active is not None : querydata['active'] = active if deleted is not None : querydata['deleted'] = deleted if items and items[0] >= items[-1]: querydata['items'] = items Not only is a time-waster, it messes up your diffs, forces short lines to be as long as long lines (increasing the chances you will exceed 80 columns), and fills your code with rivers of whitespace in the middle of the text. -- Steven
On Sep 9, 2019, at 17:43, Steven D'Aprano <steve@pearwood.info> wrote:
To be fair, you can use editor plugins to manage this stuff. For example, I used to have a mess of emacs lisp that I maintained that turned exactly your “oh no I have to reformat all the things” case into “ctrl-c :”. But I no longer use any of that. I’m much happier since I set up the black auto-formatter as a save hook. I can no longer do fancy things like lining up the colons, so I no longer waste time doing fancy things like lining up the colons (or writing lisp code to do it for me). Occasionally, it does turn a complicated expression into an unreadable mess, but then I just break the statement up into two statements that it can handle just fine, and the result is almost always easier to follow anyway. The only problem is that if I’m editing someone else’s code who doesn’t use black, I have to disable my save hook so I don’t accidentally commit a diff that destroys all their fancy lined up colons. Anyway, back on topic: earlier I facetiously suggested that you could simplify this code by replacing the mess of local variables with a simple namespace that holds them all, then dictifying and filtering that… but actually, I’m not sure that’s a joke anymore. Why _should_ all those things be in a whole mess of locals, if the main thing you’re using them for is to create a config dict or JSON response or whatever? They probably are conceptually attributes of some object, and if the object is too dumb to be worth declaring a custom class for it and writing a for_json or as_dict or whatever, you can just use a SimpleNamespace.
On Thu, 5 Sep 2019 at 11:19, <rls@hwyl.org> wrote:
This is pretty easy to write as a helper function:
Given this, I'm not sure it's a sufficiently useful improvement to be worth adding to the builtin dictionary (although thanks for the suggestion, I wouldn't have ever thought about writing a helper to avoid the boilerplate without this posting to prompt me!) Paul
I was thinking more in this direction: d = dict() d[action in not None and 'action'] = action d[minval > 0 and 'minval'] = minval ... del d[False] There are few things I do not particularly like about this (assigning to dict, even when value is discarded later, or necessity of the del d[False]), but I guess it is as expressive as it could be and I could imagine using it in my app easily. If needed one can even define another key instead of False.
Oooookie, looks like my code got munched in the web view -- how do I make it not do that?
How is this different to the discussion https://mail.python.org/archives/list/python-ideas@python.org/thread/MFSL3U6... ? On Thu, Sep 5, 2019 at 9:24 AM <brian.skinn@gmail.com> wrote:
-- Sebastian Kreft
On Sep 5, 2019, at 07:46, Sebastian Kreft <skreft@gmail.com> wrote:
How is this different to the discussion https://mail.python.org/archives/list/python-ideas@python.org/thread/MFSL3U6... ?
Well, that discussion started with an unworkable proposal, and generated a variety of different alternatives, some of which were workable, but all of which were buried deep in the discussion of the unworkable idea. Polishing up one of those alternatives and extracting it out into its own new thread doesn’t seem like a bad thing to do. A link back to the old thread might have been nice, but now you’ve fixed that. :)
Also, if one have in mind that dict addition with the `+` operator had been recelently approved, as of Python 3.9 it will be ok to write: ``` return ({ 'user_id': user_id,} + ({'max_results': max_results} if max_results else {}) + ({'active': active} if active is not None else {}) + ({'deleted': deleted} if deleted is not None else {}) ) And actually, on a second though, this is already valid as of current Python - and is potentially more readable than a magic "dict.skip" singleton. ``` return { 'user_id': user_id, **({'max_results': max_results} if max_results else {}), **({'active': active} if active is not None else {}), **({'deleted': deleted} if deleted is not None else {}) } ``` On Thu, 5 Sep 2019 at 10:22, <brian.skinn@gmail.com> wrote:
On Thu, Sep 5, 2019 at 8:37 PM Joao S. O. Bueno <jsbueno@python.org.br> wrote:
Also, if one have in mind that dict addition with the `+` operator had been recelently approved
Where did you hear this? I am not aware that this was even made into a PEP let alone that such a PEP was approved. -- --Guido van Rossum (python.org/~guido) *Pronouns: he/him/his **(why is my pronoun here?)* <http://feministing.com/2015/02/03/how-using-they-as-a-singular-pronoun-can-c...>
On Fri, 6 Sep 2019 at 01:54, Guido van Rossum <guido@python.org> wrote:
Well - the idea showed up here, and got an immediate "that is nice, let's do it" from you, followed by approval from a lot more people, and very little controverse. So, it is pretty much "approved", even if not yet detailed in an approved PEP. And 3.9 still sounds quite realistic for that specific feature, unless no one actually writes the PEP. But my bad, I thought the full approval process (PEP included) for that had been carried on already. Nonetheless, the mapping expansion syntax do allow for the OP's needs, and is even a bit shorter than the version using "+".
On Sat, Sep 07, 2019 at 12:35:09AM -0300, Joao S. O. Bueno wrote:
As the PEP author, I don't consider it even close to approved. I'm still slowly working my way through the next draft of the PEP. There are lots of objections and unresolved questions: - is this feature necessary? - if so, should it be an operator, a dict method or a function? - if an operator, should it be spelled ``+`` or ``|`` or something else? - what precise behaviour should it have? At the time I wrote the first draft, there was no consensus that the feature was necessary, but most people who thought it would be useful agreed that ``+`` was the right operator. Since then, further discussion seemed to shift closer to consensus that the feature might be useful, but away from using ``+``. -- Steven
On Sat, 7 Sep 2019 at 01:24, Steven D'Aprano <steve@pearwood.info> wrote:
Thank you for the feedback. I will follow the PEP evolution more close now. And indeed, as we can see, even in a case like this in the thread, where the "+" could have been useful (1) the OP did not think of using it at all, coming up with a different approach, and (2) the exiting star-map expansion can do what "+" would do in this case. These would point to that maybe it is not "necessary" at all. Although I still think it would be a useful feature, and, on that, maybe keeping the consistence with the "|" already in use by 'set' would be better.
On Sep 4, 2019, at 15:25, rls@hwyl.org wrote:
..."skip" being a unique object defined in and accessible from dict (there may be better places to make it available). The dict insertion code then simply ignores those entries when creating the dict.
I’m a bit confused about exactly when this does and doesn’t come into play. You say here “the dict insertion code” does it. Would they mean d[key]=d.skip doesn’t insert anything? If so, then how could I put skip in a dict at all? If there’s already a d[key] does that get left alone, or deleted? Wouldn’t this even affect dict inserts that happen behind the scenes, like any time you tried to assign dict.skip to a global variable or a class or instance attribute, or pass it as a keyword argument? If an implementation doesn’t use dict.insert to build dict literals, does that mean it doesn’t work in dict literals on that implementation, even though that was the whole motivation for the feature? Maybe instead of starting with how it could be implemented, start with where you ideally do and don’t want it to take effect, and then we can try to work out how it could be implemented to do that, and how to specify it to require such an implementation. * Obviously you want it in non-comprehension dict displays, because that’s your example. * What about comprehensions? * Constructor calls with the keyword-arg API like dict(a=1, b=dict.skip, **d2}? * Constructor calls from iterables of pairs, like dict((1, 2), (3, dict.skip))? * Subscription assignments? * Calls to insert? * Calls to update? * Capturing ** keyword args? * Assignments to globals, attributes, etc. when those use a dict under the covers (as they usually do!? * Assignments to such things when they don’t use a dict (as with a slots class instance)? Also, what should happen with this case: flag0, flag1 = True, False d = {'k': 0 if flag0 else dict.skip, 'k': 1 if flag1 else dict.skip} Do we keep the last value for 'k' as usual, and because that’s dict.skip, therefore d['k'] ends up as a KeyError? Or do we keep the last non-skip value and therefore d['k'] ends up as 0? What about this: class D(dict): skip = 0 d = D() d.skip = 1 d[0] = 0 d[1] = 1 d[dict.skip] = dict.skip Do we skip the one that uses d.skip, the one that uses type(d).skip (like a special method lookup), or the one that uses dict.skip?
On Thu, 5 Sep 2019 at 16:51, Andrew Barnert <abarnert@yahoo.com> wrote:
You say here “the dict insertion code” does it. Would they mean d[key]=d.skip doesn’t insert anything?
Yes. If so, then how could I put skip in a dict at all? The intention is that you couldn't. This could raise some implementation issues!
If there’s already a d[key] does that get left alone, or deleted?
Left alone.
This was what I was attempting to do. I wasn't suggesting the way I wrote was the way it should be implemented, just a sketch of how it might look.
Yes.
* Constructor calls with the keyword-arg API like dict(a=1, b=dict.skip, **d2}?
I'd expect the b value to not be assigned.
* Constructor calls from iterables of pairs, like dict((1, 2), (3, dict.skip))?
I'd expect the 3 key to be unassigned (assuming an extra pair of brackets).
* Subscription assignments?
I'm sorry, I'm not sure what you're referring to here. Do mean as in x[dict.skip] = y or x[y] = dict.skip? If so, I'd expect both operations to be, well, skipped.
* Calls to insert? * Calls to update?
Yes. Inserts to remain unassigned, updates to keep the original value. The idea is to skip the operation.
* Capturing ** keyword args?
Yes.
* Assignments to globals, attributes, etc. when those use a dict under the covers (as they usually do!?
I'm not sure... setting an attribute/global to "dict.skip" might not have any effect. I don't think the user could complain about surprise for doing something like this.
I'd expect the assignments to happen in order, so d['k'] would be set to 0 and then the second assignment to be ignored.
This seems like a fairly pathological case, and I'm not sure it's really in the spirit of your request to focus on the use rather than the implementation! But I'd expect the key value of dict.skip to cause the entry to be skipped. Thanks for coming up with so many angles on this; it's interesting exercise to consider how many things could be affected by this. Writing a ConditionalDict type in a similar spirit would be a lot simpler, though less convenient to use. I will experiment. Thanks Rich
On 04/09/2019 23:25:32, rls@hwyl.org wrote:
if max_results > 0 : query['max_results'] = max_results if active is not None : query['active'] = active if deleted is not None: query['deleted'] = deleted If this comes out ragged in your browser, the colons and equal signs are meant to be vertically aligned. Rob Cliffe
On Sat, Sep 7, 2019 at 11:27 PM Rob Cliffe via Python-ideas <python-ideas@python.org> wrote:
I'm not bothered by putting an if and a simple statement together, but I am definitely bothered by the need to vertically align them, and especially by the triple repeated name. Not a fan of this style - looks like a bug magnet and maintenance burden. ChrisA
On 07/09/2019 18:59:49, Chris Angelico wrote:
if max_results > 0: query['max_results'] = max_results if active is not None: query['active'] = active if deleted is not None: query['deleted'] = deleted is perfectly reasonable and has the same triple repeated name. Would you rewrite it differently, and if so, how? I'm just saying that aligning similar elements and bringing them together, without intervening lines, makes it easier to see the code structure and spot typos. Not to mention using less vertical screen space. I don't understand why you think it is a bug magnet and maintenance burden. I think it is *easier* to maintain: in which of the following is it easier to spot the mistake? # Version 1: if max_results > 0 : querydata['max_results'] = max_results if active is not None : querydata['active'] = active if deleted is not None: quervdata['deleted'] = deleted # Version 2: if max_results > 0: querydata['max_results'] = max_results if active is not None: querydata['active'] = active if deleted is not None: quervdata['deleted'] = deleted Data point: I have used this style repeatedly in my code (only where appropriate, of course) and find it helpful. Rob Cliffe
On Mon, Sep 9, 2019 at 4:13 AM Rob Cliffe via Python-ideas <python-ideas@python.org> wrote:
Now add a fourth one, for result_count. What does your diff look like? Do you have to edit every other line just to realign it? That's the maintenance burden - fiddling around with formatting. Honestly, I couldn't see the mistake in _either_ version until I looked pretty closely, upon which it was equally visible in both. If I were using this in a full app, it would be something a linter could easily spot, again regardless of the alignment. You're right that the triple repeated name is not caused by your style. However, your suggestion also doesn't solve it. What I'd probably want to do, here, is build the dictionary with everything, and then have a separate pass that removes anything that's None (or maybe "any of these keys, if the value is None"), and then just special-case the requirement for max_results to be positive. But I'd be looking to make a broader change somewhere, taking into account a lot more code, rather than doing this whole "take local names and plonk them into a dict" thing at all. ChrisA
On Sep 8, 2019, at 11:34, Chris Angelico <rosuav@gmail.com> wrote:
If you really want to avoid repeating names, the only plausible option seems to be putting them in a namespace that you can then filter and dictify, instead of storing them as a bunch of separate locals. Well, I suppose you could use locals itself as that namespace: query_data = {k: v for (k, v) in locals().items() if k in {'max_results', 'active', 'deleted'} and v} … but that’s pretty horrible. And I suppose there is something Python could do to alleviate the repetitions. The stalled proposal for magic keyword arguments would let you write something like this: query_data = dict(max_results=, active=, deleted=) … which you could then filter. But I don’t think most people liked that proposal, and I don’t think this is a good enough use case to revive it.
On 08/09/2019 19:34:33, Chris Angelico wrote: they look too similar: if max_results > 0 : querydata['max_results'] = max_results if active is not None : querydata['active'] = active if deleted is not None: querodata['deleted'] = deleted Can you honestly say it's not easier to spot the inconsistency when elements that should be identical are adjacent? (BTW I changed the identifier from query to querydata because my spell-checker, and maybe yours, highlights the mistake instantly in both versions.) Rob Cliffe
On Sat, Sep 07, 2019 at 08:22:58PM +0100, Rob Cliffe via Python-ideas wrote:
Obviously the mistake is that max_results is supposed to be greater than OR EQUAL to zero. *wink* But seriously, most coding errors aren't simple spelling errors. If they are, chances are that you will immediately get a NameError, or (as you point out yourself) your spell checker will spot it. The insidious errors are logic errors and this does not help with those unless your lines are almost identical: if seq[0] == 1: query[0] == 0 if seq[1] == 1: query[1] == 1 if seq[2] == 1: query[2] == 2 in which case you should probably use a for loop. If this style works for you, I'm glad for you and don't wish to persuade you differently. But for me, I am not comfortable with this style and would not use it because I have seen far too many people (including myself) waste too much time farting about fixing formatting and alignment issues instead of doing productive work. # Oh no now I have to re-align ALL THE THINGS if max_results > 0 : querydata['max_results'] = max_results if active is not None : querydata['active'] = active if deleted is not None: querydata['deleted'] = deleted if items and items[0] >= items[-1]: querydata['items'] = items if max_results > 0 : querydata['max_results'] = max_results if active is not None : querydata['active'] = active if deleted is not None : querydata['deleted'] = deleted if items and items[0] >= items[-1]: querydata['items'] = items Not only is a time-waster, it messes up your diffs, forces short lines to be as long as long lines (increasing the chances you will exceed 80 columns), and fills your code with rivers of whitespace in the middle of the text. -- Steven
On Sep 9, 2019, at 17:43, Steven D'Aprano <steve@pearwood.info> wrote:
To be fair, you can use editor plugins to manage this stuff. For example, I used to have a mess of emacs lisp that I maintained that turned exactly your “oh no I have to reformat all the things” case into “ctrl-c :”. But I no longer use any of that. I’m much happier since I set up the black auto-formatter as a save hook. I can no longer do fancy things like lining up the colons, so I no longer waste time doing fancy things like lining up the colons (or writing lisp code to do it for me). Occasionally, it does turn a complicated expression into an unreadable mess, but then I just break the statement up into two statements that it can handle just fine, and the result is almost always easier to follow anyway. The only problem is that if I’m editing someone else’s code who doesn’t use black, I have to disable my save hook so I don’t accidentally commit a diff that destroys all their fancy lined up colons. Anyway, back on topic: earlier I facetiously suggested that you could simplify this code by replacing the mess of local variables with a simple namespace that holds them all, then dictifying and filtering that… but actually, I’m not sure that’s a joke anymore. Why _should_ all those things be in a whole mess of locals, if the main thing you’re using them for is to create a config dict or JSON response or whatever? They probably are conceptually attributes of some object, and if the object is too dumb to be worth declaring a custom class for it and writing a for_json or as_dict or whatever, you can just use a SimpleNamespace.
participants (13)
-
Andrew Barnert -
Brandt Bucher -
brian.skinn＠gmail.com -
Chris Angelico -
Guido van Rossum -
Joao S. O. Bueno -
Paul Moore -
Rich Smith -
Richard Musil -
rls＠hwyl.org
-
Rob Cliffe -
Sebastian Kreft -
Steven D'Aprano