Improving fn(arg=arg, name=name, wibble=wibble) code
I thank Steve D'Aprano for pointing me to this real-life (although perhaps extreme) code example https://github.com/Tinche/aiofiles/blob/master/aiofiles/threadpool/__init__.... <code> def open(file, mode='r', buffering=-1, encoding=None, errors=None, newline=None, closefd=True, opener=None, *, loop=None, executor=None): return AiofilesContextManager(_open(file, mode=mode, buffering=buffering, encoding=encoding, errors=errors, newline=newline, closefd=closefd, opener=opener, loop=loop, executor=executor)) @asyncio.coroutine def _open(file, mode='r', buffering=-1, encoding=None, errors=None, newline=None, closefd=True, opener=None, *, loop=None, executor=None): """Open an asyncio file.""" if loop is None: loop = asyncio.get_event_loop() cb = partial(sync_open, file, mode=mode, buffering=buffering, encoding=encoding, errors=errors, newline=newline, closefd=closefd, opener=opener) f = yield from loop.run_in_executor(executor, cb) return wrap(f, loop=loop, executor=executor) </code> Anders Hovmöller has proposed a Python syntax extension to improve this code. It provides, for example return wrap(f, *, loop, executor) as a shorthand for return wrap(f, loop=loop, executor=executor) See: https://mail.python.org/pipermail/python-ideas/2018-September/053207.html I'd like us, in this thread, to discuss OTHER possible ways of improving this code. This could include refactoring, and the introduction of tools. I'm particularly interested in gathering alternatives, and at this time not much interesting in "knowing which one is best". -- Jonathan
It’s obvious but there is one easy way to shorten the code: using **kwargs. It’s way shorter but the down sides are: - the “real” function signature gets hidden so IDEs for example won’t pick it up - the error when you make a mistake when calling is not in your code anymore but one level down. This is confusing. One could imagine solving this specific case by having a type annotation of “this function has the types of that function”. Maybe: def _open(*args: args_of_(sync_open), **kwargs: kwargs_of(sync_open) -> return_of(sync_open): But of course this only solves the case where there is a 1:1 mapping. / Anders
On 8 Sep 2018, at 13:17, Jonathan Fine <jfine2358@gmail.com> wrote:
I thank Steve D'Aprano for pointing me to this real-life (although perhaps extreme) code example
https://github.com/Tinche/aiofiles/blob/master/aiofiles/threadpool/__init__.... <code> def open(file, mode='r', buffering=-1, encoding=None, errors=None, newline=None, closefd=True, opener=None, *, loop=None, executor=None): return AiofilesContextManager(_open(file, mode=mode, buffering=buffering, encoding=encoding, errors=errors, newline=newline, closefd=closefd, opener=opener, loop=loop, executor=executor))
@asyncio.coroutine def _open(file, mode='r', buffering=-1, encoding=None, errors=None, newline=None, closefd=True, opener=None, *, loop=None, executor=None): """Open an asyncio file.""" if loop is None: loop = asyncio.get_event_loop() cb = partial(sync_open, file, mode=mode, buffering=buffering, encoding=encoding, errors=errors, newline=newline, closefd=closefd, opener=opener) f = yield from loop.run_in_executor(executor, cb)
return wrap(f, loop=loop, executor=executor) </code>
Anders Hovmöller has proposed a Python syntax extension to improve this code. It provides, for example return wrap(f, *, loop, executor) as a shorthand for return wrap(f, loop=loop, executor=executor)
See: https://mail.python.org/pipermail/python-ideas/2018-September/053207.html
I'd like us, in this thread, to discuss OTHER possible ways of improving this code. This could include refactoring, and the introduction of tools. I'm particularly interested in gathering alternatives, and at this time not much interesting in "knowing which one is best".
-- Jonathan _______________________________________________ Python-ideas mailing list Python-ideas@python.org https://mail.python.org/mailman/listinfo/python-ideas Code of Conduct: http://python.org/psf/codeofconduct/
On Sat, Sep 8, 2018 at 10:21 PM, Anders Hovmöller <boxed@killingar.net> wrote:
It’s obvious but there is one easy way to shorten the code: using **kwargs. It’s way shorter but the down sides are:
- the “real” function signature gets hidden so IDEs for example won’t pick it up - the error when you make a mistake when calling is not in your code anymore but one level down. This is confusing.
One could imagine solving this specific case by having a type annotation of “this function has the types of that function”. Maybe:
def _open(*args: args_of_(sync_open), **kwargs: kwargs_of(sync_open) -> return_of(sync_open):
But of course this only solves the case where there is a 1:1 mapping.
That can be done with functools.wraps(). ChrisA
On 9/8/2018 7:17 AM, Jonathan Fine wrote:
I thank Steve D'Aprano for pointing me to this real-life (although perhaps extreme) code example
https://github.com/Tinche/aiofiles/blob/master/aiofiles/threadpool/__init__.... <code> def open(file, mode='r', buffering=-1, encoding=None, errors=None, newline=None, closefd=True, opener=None, *, loop=None, executor=None): return AiofilesContextManager(_open(file, mode=mode, buffering=buffering, encoding=encoding, errors=errors, newline=newline, closefd=closefd, opener=opener, loop=loop, executor=executor))
Given that open and _open, likely written at the same time, have the same signature, I would have written the above as the slightly faster call return AiofilesContextManager(_open( file, mode, buffering, encoding, errors, newline, closefd, opener, loop=loop, executor=executor))
@asyncio.coroutine def _open(file, mode='r', buffering=-1, encoding=None, errors=None, newline=None, closefd=True, opener=None, *, loop=None, executor=None): """Open an asyncio file.""" if loop is None: loop = asyncio.get_event_loop() cb = partial(sync_open, file, mode=mode, buffering=buffering, encoding=encoding, errors=errors, newline=newline, closefd=closefd, opener=opener) f = yield from loop.run_in_executor(executor, cb)
return wrap(f, loop=loop, executor=executor) </code>
-- Terry Jan Reedy
OK. Here's another piece of code to look at, from a URL that Kyle Lahnakoski posted to a different thread. <code> https://github.com/django/django/blob/master/django/contrib/admin/options.py... def get_inline_formsets(self, request, formsets, inline_instances, obj=None): inline_admin_formsets = [] for inline, formset in zip(inline_instances, formsets): fieldsets = list(inline.get_fieldsets(request, obj)) readonly = list(inline.get_readonly_fields(request, obj)) has_add_permission = inline._has_add_permission(request, obj) has_change_permission = inline.has_change_permission(request, obj) has_delete_permission = inline.has_delete_permission(request, obj) has_view_permission = inline.has_view_permission(request, obj) prepopulated = dict(inline.get_prepopulated_fields(request, obj)) inline_admin_formset = helpers.InlineAdminFormSet( inline, formset, fieldsets, prepopulated, readonly, model_admin=self, has_add_permission=has_add_permission, has_change_permission=has_change_permission, has_delete_permission=has_delete_permission, has_view_permission=has_view_permission, ) inline_admin_formsets.append(inline_admin_formset) return inline_admin_formsets </code> How can we make this code better? Again, ideas please, and no discussion of "which is best". (We can get to that later.) -- Jonathan
The problem is to improve https://github.com/django/django/blob/master/django/contrib/admin/options.py... Here's a suggestion. For each of four permissions, the code has an assignment such as has_add_permission = inline._has_add_permission(request, obj) and the function call has four named parameters such as has_add_permission=has_add_permission where the permissions are add, change, deleted and view. Browsing the file, I see something similar in several places, such as <code> https://github.com/django/django/blob/master/django/contrib/admin/options.py... 'has_view_permission': self.has_view_permission(request, obj), 'has_add_permission': self.has_add_permission(request), 'has_change_permission': self.has_change_permission(request, obj), 'has_delete_permission': self.has_delete_permission(request, obj), </code> So, off the top of my head, have a function get_permissions(item, request, obj) and then simply write ** get_permissions(item, request, obj) in the function call, to pass the permissions to the called function. By the way, for ease of use this is relying on https://www.python.org/dev/peps/pep-0448/ # Additional Unpacking Generalizations which allows multiple **kwargs in a function call. It was implemented in Python 3.5. By the way, Django 1.11 and 2.0 are still supported by Django, and they both support Python 3.4. So we'll have to wait for a bit before Django could accept this suggestion. https://docs.djangoproject.com/en/2.1/faq/install/#faq-python-version-suppor... -- Jonathan
get_permissions(item, request, obj)
and then simply write
** get_permissions(item, request, obj)
in the function call, to pass the permissions to the called function. By the way, for ease of use this is relying on
https://www.python.org/dev/peps/pep-0448/ # Additional Unpacking Generalizations
which allows multiple **kwargs in a function call. It was implemented in Python 3.5.
By the way, Django 1.11 and 2.0 are still supported by Django, and they both support Python 3.4. So we'll have to wait for a bit before Django could accept this suggestion.
A dict merge is fairly trivial to implement to get even 2.7 support so no need to be that restrictive. This is a good fix for this case.
Hi Anders You wrote:
A dict merge is fairly trivial to implement to get even 2.7 support so no need to be that restrictive. This is a good fix for this case.
I very much appreciate your openness to solutions other than the one you proposed. I like experts who are willing to change their view, when presented with new evidence. Thank you. -- Jonathan
On Sat, Sep 8, 2018 at 4:17 AM Jonathan Fine <jfine2358@gmail.com> wrote:
I thank Steve D'Aprano for pointing me to this real-life (although perhaps extreme) code example
https://github.com/Tinche/aiofiles/blob/master/aiofiles/threadpool/__init__....
It's hard to know from just a brief glance how the "private" _open function will be used. Using GitHub's repo search, it looks like it's only called once and only in this module. Since it has essentially the same signature as the "public" open function, why not just use *args and **kwds for the private _open? Here's a quick refactor that emphasizes the pass-through relationship of open and _open: def open(file, mode='r', buffering=-1, encoding=None, errors=None, newline=None, closefd=True, opener=None, *, loop=None, executor=None): return AiofilesContextManager(_open(**locals())) @asyncio.coroutine def _open(file, *args, **kwds): """Open an asyncio file.""" executor = kwds.pop('executor') loop = kwds.pop('loop') if loop is None: loop = asyncio.get_event_loop() callback = partial(sync_open, file, *args, **kwds) f = yield from loop.run_in_executor(executor, callback) return wrap(f, loop=loop, executor=executor) I normally dislike the use of **locals(), but here it helps make explicit that the "public" open function passes all arguments through to its helper.
Summary: Michael Selik has produced a nice refactoring of an example. I suggest further refactoring, to create a function decorator that does the job. This might be useful if the example is an instance of a common use pattern. Michael Selik wrote
def open(file, mode='r', buffering=-1, encoding=None, errors=None, newline=None, closefd=True, opener=None, *, loop=None, executor=None): return AiofilesContextManager(_open(**locals()))
I normally dislike the use of **locals(), but here it helps make explicit that the "public" open function passes all arguments through to its helper.
Here's the call signature for the built-in open >>> help(open) open(file, mode='r', buffering=-1, encoding=None, errors=None, newline=None, closefd=True, opener=None) And for the new open Same as builtin open, with (keyword-only) loop=None, executor=None added. To explore, let's refactor, as if this were a common use pattern. Your code, Michael, (almost) reduces the definition of the new open function to A signature A function to call on the resulting locals() So how would we refactor this. I suggest (not tested) @wibble(sig) def new_open(kwargs): return AiofilesContextManager(_open(**kwargs)) The sig argument to the decorator wibble could be an instance of https://docs.python.org/3/library/inspect.html#inspect.Signature The semantics of wibble should be such that the above is equivalent to Michael's code, quoted at the top of this post. Finally, we'd like some easy way of combining the signature of (the built-in) open with the additional loop and executor parameters. I see two questions arising. First, can this conveniently implemented using Python as it is? Second, does the decorator wibble (perhaps enhanced) help with common use patterns. I'm grateful to you, Michael, for your nice clear example of refactoring. Given the narrow context of the problem, I don't see how it could be bettered. But if it's a common use pattern, refactoring into a decorator might be good. Would anyone here like to try coding up a proof-of-concept decorator? Or explore code-bases for this and similar use patterns? I'm busy to the end of this month, so have little time myself. -- Jonathan
Summary: Michael Selik has produced a nice refactoring of an example. I suggest further refactoring, to create a function decorator that does the job. This might be useful if the example is an instance of a common use pattern.
It seems to me this discussion has drifted away from the original discussion toward one where you have a chain of functions with the same or almost the same signature. This is interesting for sure but we shouldn’t forget about the original topic: how can we make it less painful to use keyword arguments? Just my two cents. / Anders
On Thu, Sep 13, 2018, 12:39 AM Anders Hovmöller <boxed@killingar.net> wrote:
It seems to me this discussion has drifted away from the original discussion toward one where you have a chain of functions with the same or almost the same signature. This is interesting for sure but we shouldn’t forget about the original topic: how can we make it less painful to use keyword arguments?
Using keyword arguments is not painful. It's ugly in some unusual cases, such as creating helper functions with nearly the same signature. I try to avoid that situation in my own code. It sometimes requires a significant design change, but I'm usually pleased with the results. If you don't have the inclination to reconsider the design, it could be nice to create a standard code pattern for these pass-through functions to make them more beautiful.
Using keyword arguments is not painful. It's ugly in some unusual cases, such as creating helper functions with nearly the same signature.
It’s more painful than positional. To me the fact that everyone who works on code bases that are of non-trivial size see positional arguments being used for calls with more than 3 arguments is a pretty obvious proof. I know I write them myself because it’s nicer even though you’ll have a hard time finding someone who is more of a proponent for kwargs everywhere than me! I feel this pain daily. You aren’t me so you can’t say if I feel this or not.
I try to avoid that situation in my own code. It sometimes requires a significant design change, but I'm usually pleased with the results. If you don't have the inclination to reconsider the design, it could be nice to create a standard code pattern for these pass-through functions to make them more beautiful.
Not talking about those. But I agree that fixing those might be a good first step. Unless it hinders the progress of the bigger issue of course. I’ll repeat myself: what about .format()? If you localize you can’t use f-strings. What about templates in web apps? Obviously f-strings won’t do. What about json blobs in REST APIs? Again no help from f-strings. What about functions with more than 3 arguments generally? / Anders
Hi Anders and Michael I think it would be great if plugging existing functions into our code was as easy as, well, plugging an electrical appliance into a wall socket. However, even this ease of use has taken time. https://en.wikipedia.org/wiki/AC_power_plugs_and_sockets#History And travellers know that adapters are required. And for some equipment, non-trivial transformers. Even though there's been much progress, such as multiple inheritance, super() and iterators, there's still nuisance, pain and errors.The good news is that we've identified some ways of improving the situation. Although there are the usual contending views and differences of opinion, there's also positive energy and shared values. So I'm optimistic. Anders, you wrote:
I’ll repeat myself: what about .format()? If you localize you can’t use f-strings. What about templates in web apps? Obviously f-strings won’t do. What about json blobs in REST APIs? Again no help from f-strings. What about functions with more than 3 arguments generally?
For f-strings, can't we turn the dictionary into a namespace, like so Python 3.6.2 (default, Jul 29 2017, 00:00:00) >>> d = dict(a='apple', b='banana', c='cherry', d=137) >>> class A: pass >>> ns = A() >>> ns.__dict__ = d # Yes, we can do this! >>> ns.a 'apple' >>> f'The a-fruit is {ns.a}, and d is {ns.d}.' 'The a-fruit is apple, and d is 137.' I don't see why this doesn't help reduce the pain. For functions with more than 3 arguments, perhaps you could give some examples where you'd like improvement. best wishes Jonathan
I’ll repeat myself: what about .format()? If you localize you can’t use f-strings. What about templates in web apps? Obviously f-strings won’t do. What about json blobs in REST APIs? Again no help from f-strings. What about functions with more than 3 arguments generally?
For f-strings, can't we turn the dictionary into a namespace, like so
I say I’m talking about cases where you can’t use f-strings and you directly talk about f-strings… I think you might have missed the point.
For functions with more than 3 arguments, perhaps you could give some examples where you'd like improvement.
Sure. Run this script against django: https://gist.github.com/boxed/e60e3e19967385dc2c7f0de483723502 <https://gist.github.com/boxed/e60e3e19967385dc2c7f0de483723502> It will print all function calls that are positional and have > 2 arguments. Not a single one is good as is, all would be better with keyword arguments. A few of them are pass through as discussed before, but far from all. For example: django-master/django/http/multipartparser.py 225 handler.new_file( field_name, file_name, content_type, content_length, charset, content_type_extra, ) That’s positional because keyword is more painful. / Anders
On Thu, Sep 13, 2018 at 5:22 PM Anders Hovmöller <boxed@killingar.net> wrote:
For example: django-master/django/http/multipartparser.py 225
Sorry, I didn't recognize this as a link on first read. I'll provide a link here to the code in context. https://github.com/django/django/blob/e7a0a5c8b21f5ad1a0066bd0dfab84466b474e... This is a fairly large function that might benefit from being refactored to clarify the code. On the other hand, I don't advocate creating too many helper functions with only one callsite. The one thing that catches my eye is the repeated use of ``exhaust`` in the else clause to several layers of the nested try and if blocks. It's a bit too large for me to make sense of it quickly. My apologies for not offering a holistic refactor. That’s positional because keyword is more painful.
Why would keyword arguments be more painful here? They've already split the call across 4 lines. Why not go a bit further and use keyword args to make it 6 or 7 lines? Maybe they decided it reads fine as it is. Sure. Run this script against django:
https://gist.github.com/boxed/e60e3e19967385dc2c7f0de483723502
It will print all function calls that are positional and have > 2 arguments. Not a single one is good as is, all would be better with keyword arguments.
I disagree. Please expand your assertion by explaining why an example is not good as-is and would be better with keyword arguments.
It's a bit too large for me to make sense of it quickly. My apologies for not offering a holistic refactor.
My tool will print plenty of other examples. You can pick anyone really...
That’s positional because keyword is more painful.
Why would keyword arguments be more painful here? They've already split the call across 4 lines. Why not go a bit further and use keyword args to make it 6 or 7 lines? Maybe they decided it reads fine as it is.
Yes, exactly. Why not go further? This is exactly my point.
Sure. Run this script against django: https://gist.github.com/boxed/e60e3e19967385dc2c7f0de483723502 <https://gist.github.com/boxed/e60e3e19967385dc2c7f0de483723502>
It will print all function calls that are positional and have > 2 arguments. Not a single one is good as is, all would be better with keyword arguments.
I disagree. Please expand your assertion by explaining why an example is not good as-is and would be better with keyword arguments.
Because keyword arguments are checked to correspond to the parameters. The example I gave was: handler.new_file( field_name, file_name, content_type, content_length, charset, content_type_extra, ) it reads fine precisely because the variables names match with the signature of new_file(). But if new_file() is changed they won't match up anymore and it will still read fine and look ok, but now the parameters don't line up and it's broken in potentially very subtle ways. For example if content_type and file_name switch places. Those are the same time but the consequences for the mixup are fairly large and potentially very annoying to track down. Do you disagree on this point? / Anders
On Thu, Sep 13, 2018, 11:48 PM Anders Hovmöller <boxed@killingar.net> wrote:
It's a bit too large for me to make sense of it quickly. My apologies for not offering a holistic refactor.
My tool will print plenty of other examples. You can pick anyone really...
That’s positional because keyword is more painful.
Why would keyword arguments be more painful here? They've already split the call across 4 lines. Why not go a bit further and use keyword args to make it 6 or 7 lines? Maybe they decided it reads fine as it is.
Yes, exactly. Why not go further? This is exactly my point.
Sure. Run this script against django:
https://gist.github.com/boxed/e60e3e19967385dc2c7f0de483723502
It will print all function calls that are positional and have > 2 arguments. Not a single one is good as is, all would be better with keyword arguments.
I disagree. Please expand your assertion by explaining why an example is not good as-is and would be better with keyword arguments.
Because keyword arguments are checked to correspond to the parameters. The example I gave was:
handler.new_file( field_name, file_name, content_type, content_length, charset, content_type_extra, )
it reads fine precisely because the variables names match with the signature of new_file(). But if new_file() is changed they won't match up anymore and it will still read fine and look ok, but now the parameters don't line up and it's broken in potentially very subtle ways. For example if content_type and file_name switch places. Those are the same time but the consequences for the mixup are fairly large and potentially very annoying to track down.
Do you disagree on this point?
The mixup you describe would probably cause an immediate error as the filename would not be a valid content type. I suspect it'd be easy to track down. Keyword arguments are especially important when the type and value of two arguments are hard to distinguish, like a height and width. If one is an int and the other is a str, or if it must be a str with only a handful of valid values, I'm not so worried about making mistakes. Further, I think it's reasonable to use keyword arguments here and no more "painful" than the use of positional arguments in this case. Both are annoyingly verbose. I'd prefer refactoring the new_file method, but I didn't spot where it's defined.
it reads fine precisely because the variables names match with the signature of new_file(). But if new_file() is changed they won't match up anymore and it will still read fine and look ok, but now the parameters don't line up and it's broken in potentially very subtle ways. For example if content_type and file_name switch places. Those are the same time but the consequences for the mixup are fairly large and potentially very annoying to track down.
Do you disagree on this point?
The mixup you describe would probably cause an immediate error as the filename would not be a valid content type. I suspect it'd be easy to track down.
So no you don't agree?
Keyword arguments are especially important when the type and value of two arguments are hard to distinguish, like a height and width. If one is an int and the other is a str, or if it must be a str with only a handful of valid values, I'm not so worried about making mistakes.
Further, I think it's reasonable to use keyword arguments [...]
...now I'm confused. Now it sounds like you do agree?
[...] here and no more "painful" than the use of positional arguments in this case. Both are annoyingly verbose.
That's a bit of a dodge. There is a huge difference in verbosity between handler.new_file(field_name, file_name, content_type, content_length, charset, content_type_extra) and handler.new_file(field_name=field_name, file_name=file_name, content_type=content_type, content_length=content_length, charset=charset, content_type_extra=content_type_extra) and it's pretty obvious when it's spelled out. Now compare to my two suggested syntaxes: handler.new_file(*, field_name, file_name, content_type, content_length, charset, content_type_extra) handler.new_file(=field_name, =file_name, =content_type, =content_length, =charset, =content_type_extra)
I'd prefer refactoring the new_file method, but I didn't spot where it's defined.
People keep saying that, but I don't buy it. Refactoring isn't magic, it won't just make the data required to a function go away. I've been pressed to get hard numbers, I have. I've been pressed to come up with actual examples, I have. Now when I have a point you're just waving your hand and saying "refactoring". I don't think that's an actual argument. / Anders
On Fri, Sep 14, 2018, 12:17 AM Anders Hovmöller <boxed@killingar.net> wrote:
That's a bit of a dodge. There is a huge difference in verbosity between
handler.new_file(field_name, file_name, content_type, content_length, charset, content_type_extra)
and
handler.new_file(field_name=field_name, file_name=file_name, content_type=content_type, content_length=content_length, charset=charset, content_type_extra=content_type_extra)
Since neither version fits well on one line or even three, I'd have written each of those on a separate line, indented nicely to emphasize the repetition. Seems fine to me. The real problem with this code section wasn't the positional arguments, but that it was so heavily indented. Whenever I have code so nested, I put in a great deal of effort to refactor so that I can avoid the nesting of many try/except, for, and if/else statements.
On 14 Sep 2018, at 09:26, Michael Selik <mike@selik.org> wrote:
On Fri, Sep 14, 2018, 12:17 AM Anders Hovmöller <boxed@killingar.net <mailto:boxed@killingar.net>> wrote:
That's a bit of a dodge. There is a huge difference in verbosity between
handler.new_file(field_name, file_name, content_type, content_length, charset, content_type_extra)
and
handler.new_file(field_name=field_name, file_name=file_name, content_type=content_type, content_length=content_length, charset=charset, content_type_extra=content_type_extra)
Since neither version fits well on one line or even three, I'd have written each of those on a separate line, indented nicely to emphasize the repetition. Seems fine to me.
Sure. As would I. Doesn't change anything: handler.new_file( field_name=field_name, file_name=file_name, content_type=content_type, content_length=content_length, charset=charset, content_type_extra=content_type_extra, )
The real problem with this code section wasn't the positional arguments, but that it was so heavily indented. Whenever I have code so nested, I put in a great deal of effort to refactor so that I can avoid the nesting of many try/except, for, and if/else statements.
Most code has many problems. Bringing up random other problems instead of sticking to the topic doesn't seem very helpful. Or did you want to change the topic to generally be about how to clean up code? If so I'd like to know so I can stop responding, because that's not what I am trying to discuss. / Anders
On Fri, Sep 14, 2018 at 12:30 AM Anders Hovmöller <boxed@killingar.net> wrote:
Since neither version fits well on one line or even three, I'd have written each of those on a separate line, indented nicely to emphasize the repetition. Seems fine to me.
Sure. As would I. Doesn't change anything[.]
Our aesthetics are different. Some people like Rothko, others don't. Since that pattern is rare for me, I don't wish for a better way. The real problem with this code section wasn't the positional arguments,
but that it was so heavily indented. Whenever I have code so nested, I put in a great deal of effort to refactor so that I can avoid the nesting of many try/except, for, and if/else statements.
Most code has many problems. Bringing up random other problems instead of sticking to the topic doesn't seem very helpful. Or did you want to change the topic to generally be about how to clean up code? If so I'd like to know so I can stop responding, because that's not what I am trying to discuss.
I don't think it's off-topic. The ugliness of the line you're talking about stems in large part from that it's in a heavily indented complex section. The distinction between positional and keyword args in this case is superficial. On Fri, Sep 14, 2018 at 12:27 AM Anders Hovmöller <boxed@killingar.net> wrote:
I enjoy using keywords to explain the values I'm passing. If I already have a well-named variable, I'm less keen on using a keyword.
Here lies the crux for me. You're describing telling the next reader what the arguments are. But without keyword arguments you aren't telling the computer what the arguments are, not really. The position and the names become disconnected when they should be connected.
It's very similar to how in C you use {} for the computer but indent for the human, and no one checks that they are the same (not true anymore strictly because I believe clang has an option you can turn off to validate this but it's default off). In python you tell the computer and the human the same thing with the same language. This is robust and clear. I think this situation is the same.
It may be similar in theme, but in my experience it's different in practice -- how often the problem arises and how difficult it is to detect and fix. Remembering your comments about string interpolation, it sounds like you write a lot of templating code and craft large JSON blobs for REST APIs. I agree that keyword arguments are critical in those situations for avoiding tedious and error-prone positional matching. When I have similar situations I tend to avoid the `f(a=a)` annoyance by creating kwds dicts and passing them around with ** unpacking.
Here's a possible refactor. I didn't bother with keyword arguments, because the variable names are easy to match up with arguments positionally. My screen was large enough that I could read the signature at the same time as I wrote the call.
def recurse(name): return self.find_ordering_name(name, opts, alias, order,
already_seen)
return itertools.chain.from_iterable(map(recurse, opts.ordering))
I don't see how that changed anything. That just changes it to a functional style, but otherwise it's identical.
It avoids creating extra lists, which might make it more efficient. It also emphasizes the recursion, which wasn't obvious to me when I first glanced at the code. No, I didn't bother switching to keyword arguments, because I didn't feel it helped. My screen was big enough to verify the order as I wrote the arguments. On Thu, Sep 13, 2018 at 11:35 AM Anders Hovmöller <boxed@killingar.net> wrote:
I’ll repeat myself [...]
If you feel like you're repeating yourself, the easiest way to avoid frustration is to simply stop. Don't fall for the sunk cost fallacy, despite the effort you've put into this proposal. Have a good evening.
I don't think it's off-topic. The ugliness of the line you're talking about stems in large part from that it's in a heavily indented complex section. The distinction between positional and keyword args in this case is superficial.
Indenting 4 levels (32 spaces) is a problem, but adding an extra 77 non-space characters is superficial? Is this your position?
Remembering your comments about string interpolation, it sounds like you write a lot of templating code and craft large JSON blobs for REST APIs. I agree that keyword arguments are critical in those situations for avoiding tedious and error-prone positional matching.
Well no they are not. You wouldn't have positional arguments because you'd create a dict and pass it.
When I have similar situations I tend to avoid the `f(a=a)` annoyance by creating kwds dicts and passing them around with ** unpacking.
Which everyone does. But how would you create that dict?
It avoids creating extra lists, which might make it more efficient. It also emphasizes the recursion, which wasn't obvious to me when I first glanced at the code.
Sure. All good improvements. No argument from me.
No, I didn't bother switching to keyword arguments, because I didn't feel it helped. My screen was big enough to verify the order as I wrote the arguments.
This makes me very frustrated. You could rely on the machine to verify the arguments, but you spend extra time to avoid it. You're explicitly avoiding to send a machine to do a machines job. Why? I'm arguing you're doing it *because it produces shorter code*. This is exactly my point. We should send a machine to do a machines job, and if the language pushes people to send a man to do a machines job then this is bad.
If you feel like you're repeating yourself, the easiest way to avoid frustration is to simply stop. Don't fall for the sunk cost fallacy, despite the effort you've put into this proposal.
Well ok, you should have started with that. I'll just send what I've written so far :) / Anders
On Thu, Sep 13, 2018 at 11:35 AM Anders Hovmöller <boxed@killingar.net> wrote:
Using keyword arguments is not painful. It's ugly in some unusual cases, such as creating helper functions with nearly the same signature.
It’s more painful than positional. To me the fact that everyone who works on code bases that are of non-trivial size see positional arguments being used for calls with more than 3 arguments is a pretty obvious proof.
Looking through my recent code, I find I rarely call functions passing more than 3 arguments, unless using * or ** unpacking. This might be part of the difference in our styles. I feel this pain daily. You aren’t me so you can’t say if I feel this or
not.
Yes, of course. Pain is subjective. Are you aware that changing Python syntax is painful for not only the implementers, but also the users?
On 13 Sep 2018, at 21:34, Michael Selik <mike@selik.org> wrote:
On Thu, Sep 13, 2018 at 11:35 AM Anders Hovmöller <boxed@killingar.net <mailto:boxed@killingar.net>> wrote:
Using keyword arguments is not painful. It's ugly in some unusual cases, such as creating helper functions with nearly the same signature.
It’s more painful than positional. To me the fact that everyone who works on code bases that are of non-trivial size see positional arguments being used for calls with more than 3 arguments is a pretty obvious proof.
Looking through my recent code, I find I rarely call functions passing more than 3 arguments, unless using * or ** unpacking. This might be part of the difference in our styles.
I wrote a script so you can get a list of them all in big code bases without looking through the code randomly. https://gist.github.com/boxed/e60e3e19967385dc2c7f0de483723502 <https://gist.github.com/boxed/e60e3e19967385dc2c7f0de483723502>
I feel this pain daily. You aren’t me so you can’t say if I feel this or not.
Yes, of course. Pain is subjective. Are you aware that changing Python syntax is painful for not only the implementers, but also the users?
Let’s not exaggerate here. I’ve already implemented my original proposed syntax, and I’m pretty sure the =foo syntax is similarly easy (if not easier!) to implement. It took me something like one hour and I haven’t coded C for 20 years, 7 years if you count C++, and I’ve never looked at the CPython code base before. It’s not that painful to implement. As for users I don’t buy that argument much either. We aren’t optimizing for not changing the language just to make people not learn new things. See f-strings, assignment expressions, new except syntax, matrix multiplication operator. We are optimizing for the value of python. Or, that’s how I see it anyway. / Anders
On Thu, Sep 13, 2018 at 5:34 PM Anders Hovmöller <boxed@killingar.net> wrote:
I wrote a script so you can get a list of [good use cases] in big code bases without looking through the code randomly. https://gist.github.com/boxed/e60e3e19967385dc2c7f0de483723502
In that case, you should be able to link to a compelling example. If you go to the trouble of finding one, I'll take time to try to refactor it. Let’s not exaggerate [the pain of implementation].
The pain is not just time to implement, but also code review, more surface area for bugs, increased maintenance burden, and teaching users about the new feature. We aren’t optimizing for not changing the language just to make people not
learn new things.
On the contrary, I think we should be. Ceteris paribus, it's not good to churn the language. It's not some startup hoping to find a business model. The more techniques available, the less consistent Pythonic style will be. There are always trade-offs. In this case, I don't think the benefit is worth the cost.
On 14 Sep 2018, at 03:35, Michael Selik <mike@selik.org> wrote:
On Thu, Sep 13, 2018 at 5:34 PM Anders Hovmöller <boxed@killingar.net <mailto:boxed@killingar.net>> wrote: I wrote a script so you can get a list of [good use cases] in big code bases without looking through the code randomly. https://gist.github.com/boxed/e60e3e19967385dc2c7f0de483723502 <https://gist.github.com/boxed/e60e3e19967385dc2c7f0de483723502>
In that case, you should be able to link to a compelling example. If you go to the trouble of finding one, I'll take time to try to refactor it.
https://github.com/django/django/blob/master/django/db/models/sql/compiler.p... Is a pretty typical one. The full list for django looks like this: django-master/tests/model_forms/tests.py 2837 super().__new__(cls, name, bases, attrs) django-master/tests/middleware/tests.py 374 super().is_ignorable_request(request, uri, domain, referer) django-master/tests/migrations/test_operations.py 1321 operation.database_forwards(app_label, editor, project_state, first_state) django-master/tests/migrations/test_operations.py 1332 operation.database_forwards(app_label, editor, first_state, second_state) django-master/tests/migrations/test_operations.py 1337 operation.database_backwards(app_label, editor, second_state, first_state) django-master/tests/migrations/test_operations.py 2669 operation.database_forwards(app_label, editor, project_state, new_state) django-master/tests/migrations/test_operations.py 2674 operation.database_backwards(app_label, editor, new_state, project_state) django-master/tests/migrations/test_multidb.py 65 operation.database_forwards(app_label, editor, project_state, new_state) django-master/tests/migrations/test_multidb.py 72 operation.database_backwards(app_label, editor, new_state, project_state) django-master/tests/migrations/test_multidb.py 124 operation.database_forwards(app_label, editor, project_state, new_state) django-master/tests/migrations/test_multidb.py 160 operation.database_forwards(app_label, editor, project_state, new_state) django-master/tests/raw_query/tests.py 54 self.assertProcessed(model, results, expected_results, expected_annotations) django-master/tests/raw_query/tests.py 255 self.assertSuccessfulRawQuery(Author, query, authors, expected_annotations) django-master/tests/forms_tests/widget_tests/test_multiwidget.py 45 super().__init__(fields, required, widget, label, initial) django-master/tests/contenttypes_tests/test_models.py 52 ContentType.objects.get_for_models(ContentType, FooWithUrl, ProxyModel, ConcreteModel) django-master/tests/contenttypes_tests/test_models.py 63 ContentType.objects.get_for_models(ContentType, FooWithUrl, ProxyModel, ConcreteModel) django-master/tests/admin_views/admin.py 154 super().save_model(request, obj, form, change) django-master/tests/admin_views/admin.py 335 super().save_related(request, form, formsets, change) django-master/django/templatetags/i18n.py 142 translation.npgettext(message_context, singular, plural, count) django-master/django/middleware/common.py 126 self.is_ignorable_request(request, path, domain, referer) django-master/django/forms/models.py 216 super(ModelFormMetaclass, mcs).__new__(mcs, name, bases, attrs) django-master/django/forms/widgets.py 174 super(MediaDefiningClass, mcs).__new__(mcs, name, bases, attrs) django-master/django/forms/widgets.py 899 super().__init__(attrs, date_format, time_format, date_attrs, time_attrs) django-master/django/forms/forms.py 36 super(DeclarativeFieldsMetaclass, mcs).__new__(mcs, name, bases, attrs) django-master/django/core/cache/backends/filebased.py 28 self.set(key, value, timeout, version) django-master/django/core/mail/message.py 430 super().__init__( subject, body, from_email, to, bcc, connection, attachments, headers, cc, reply_to, ) django-master/django/core/management/sql.py 17 connection.ops.sql_flush(style, tables, seqs, allow_cascade) django-master/django/core/management/commands/inspectdb.py 163 self.get_meta(table_name, constraints, column_to_field_name, is_view) django-master/django/core/serializers/python.py 123 base.deserialize_m2m_values(field, field_value, using, handle_forward_references) django-master/django/core/serializers/python.py 133 base.deserialize_fk_value(field, field_value, using, handle_forward_references) django-master/django/core/files/uploadedfile.py 62 super().__init__(file, name, content_type, size, charset, content_type_extra) django-master/django/core/files/uploadedfile.py 83 super().__init__(file, name, content_type, size, charset, content_type_extra) django-master/django/test/selenium.py 21 super().__new__(cls, name, bases, attrs) django-master/django/test/testcases.py 381 self._assert_contains( response, text, status_code, msg_prefix, html) django-master/django/test/testcases.py 398 self._assert_contains( response, text, status_code, msg_prefix, html) django-master/django/test/testcases.py 619 self._assert_raises_or_warns_cm(func, cm_attr, expected_exception, expected_message) django-master/django/template/library.py 187 super().__init__(func, takes_context, args, kwargs) django-master/django/template/library.py 204 super().__init__(func, takes_context, args, kwargs) django-master/django/template/response.py 144 super().__init__(template, context, content_type, status, charset, using) django-master/django/utils/deprecation.py 48 super().__new__(cls, name, bases, attrs) django-master/django/utils/duration.py 40 '{}P{}DT{:02d}H{:02d}M{:02d}{}S'.format(sign, days, hours, minutes, seconds, ms) django-master/django/utils/http.py 176 datetime.datetime(year, month, day, hour, min, sec) django-master/django/utils/text.py 99 self._text_chars(length, truncate, text, truncate_len) django-master/django/utils/decorators.py 138 middleware.process_view(request, view_func, args, kwargs) django-master/django/utils/translation/__init__.py 95 _trans.npgettext(context, singular, plural, number) django-master/django/contrib/admin/options.py 796 self.paginator(queryset, per_page, orphans, allow_empty_first_page) django-master/django/contrib/admin/options.py 1526 self._changeform_view(request, object_id, form_url, extra_context) django-master/django/contrib/admin/options.py 1567 self.construct_change_message(request, form, formsets, add) django-master/django/contrib/admin/options.py 1597 self.get_inline_formsets(request, formsets, inline_instances, obj) django-master/django/contrib/admin/options.py 1640 self.changeform_view(request, object_id, form_url, extra_context) django-master/django/contrib/admin/widgets.py 436 self.create_option(name, option_value, option_label, selected_choices, index) django-master/django/contrib/admin/helpers.py 333 super().__init__(form, fieldsets, prepopulated_fields, readonly_fields, model_admin) django-master/django/contrib/admin/filters.py 67 super().__init__(request, params, model, model_admin) django-master/django/contrib/admin/filters.py 126 super().__init__(request, params, model, model_admin) django-master/django/contrib/admin/filters.py 169 super().__init__(field, request, params, model, model_admin, field_path) django-master/django/contrib/admin/filters.py 228 super().__init__(field, request, params, model, model_admin, field_path) django-master/django/contrib/admin/filters.py 263 super().__init__(field, request, params, model, model_admin, field_path) django-master/django/contrib/admin/filters.py 344 super().__init__(field, request, params, model, model_admin, field_path) django-master/django/contrib/admin/filters.py 382 super().__init__(field, request, params, model, model_admin, field_path) django-master/django/contrib/postgres/search.py 62 super().resolve_expression(query, allow_joins, reuse, summarize, for_save) django-master/django/contrib/postgres/search.py 65 Value(self.config).resolve_expression(query, allow_joins, reuse, summarize, for_save) django-master/django/contrib/postgres/search.py 67 self.config.resolve_expression(query, allow_joins, reuse, summarize, for_save) django-master/django/contrib/postgres/search.py 89 super().__init__(lhs, connector, rhs, output_field) django-master/django/contrib/postgres/search.py 133 super().resolve_expression(query, allow_joins, reuse, summarize, for_save) django-master/django/contrib/postgres/search.py 136 Value(self.config).resolve_expression(query, allow_joins, reuse, summarize, for_save) django-master/django/contrib/postgres/search.py 138 self.config.resolve_expression(query, allow_joins, reuse, summarize, for_save) django-master/django/contrib/postgres/search.py 169 super().__init__(lhs, connector, rhs, output_field) django-master/django/contrib/postgres/aggregates/statistics.py 19 super().resolve_expression(query, allow_joins, reuse, summarize) django-master/django/contrib/gis/geos/mutable_list.py 269 self._assign_extended_slice(start, stop, step, valueList) django-master/django/contrib/gis/db/backends/oracle/operations.py 49 super().as_sql(connection, lookup, template_params, sql_params) django-master/django/contrib/gis/db/backends/postgis/schema.py 48 super()._alter_column_type_sql(table, old_field, new_field, new_type) django-master/django/contrib/gis/db/backends/spatialite/operations.py 22 super().as_sql(connection, lookup, template_params, sql_params) django-master/django/contrib/gis/db/backends/spatialite/schema.py 138 # Alter table super().alter_db_table(model, old_db_table, new_db_table, disable_constraints) django-master/django/contrib/gis/db/models/lookups.py 83 rhs_op.as_sql(connection, self, template_params, sql_params) django-master/django/contrib/gis/db/models/aggregates.py 35 super().resolve_expression(query, allow_joins, reuse, summarize, for_save) django-master/django/http/multipartparser.py 225 handler.new_file( field_name, file_name, content_type, content_length, charset, content_type_extra, ) django-master/django/db/migrations/autodetector.py 827 self.questioner.ask_rename(model_name, rem_field_name, field_name, field) django-master/django/db/migrations/operations/models.py 400 self.database_forwards(app_label, schema_editor, from_state, to_state) django-master/django/db/migrations/operations/models.py 472 self.database_forwards(app_label, schema_editor, from_state, to_state) django-master/django/db/migrations/operations/models.py 534 self.database_forwards(app_label, schema_editor, from_state, to_state) django-master/django/db/migrations/operations/models.py 615 self.database_forwards(app_label, schema_editor, from_state, to_state) django-master/django/db/migrations/operations/fields.py 254 self.database_forwards(app_label, schema_editor, from_state, to_state) django-master/django/db/migrations/operations/special.py 41 database_operation.database_forwards(app_label, schema_editor, from_state, to_state) django-master/django/db/migrations/operations/special.py 57 database_operation.database_backwards(app_label, schema_editor, from_state, to_state) django-master/django/db/backends/ddl_references.py 109 super().__init__(table, columns, quote_name, col_suffixes) django-master/django/db/backends/postgresql/schema.py 105 super()._alter_column_type_sql(model, old_field, new_field, new_type) django-master/django/db/backends/postgresql/schema.py 119 super()._alter_field( model, old_field, new_field, old_type, new_type, old_db_params, new_db_params, strict, ) django-master/django/db/backends/postgresql/schema.py 138 super()._index_columns(table, columns, col_suffixes, opclasses) django-master/django/db/backends/oracle/creation.py 35 self._execute_test_db_creation(cursor, parameters, verbosity, keepdb) django-master/django/db/backends/oracle/creation.py 52 self._handle_objects_preventing_db_destruction(cursor, parameters, verbosity, autoclobber) django-master/django/db/backends/oracle/creation.py 62 self._execute_test_db_creation(cursor, parameters, verbosity, keepdb) django-master/django/db/backends/oracle/creation.py 74 self._create_test_user(cursor, parameters, verbosity, keepdb) django-master/django/db/backends/oracle/creation.py 91 self._create_test_user(cursor, parameters, verbosity, keepdb) django-master/django/db/backends/oracle/creation.py 202 self._execute_allow_fail_statements(cursor, statements, parameters, verbosity, acceptable_ora_err) django-master/django/db/backends/oracle/creation.py 223 self._execute_allow_fail_statements(cursor, statements, parameters, verbosity, acceptable_ora_err) django-master/django/db/backends/oracle/creation.py 241 self._execute_statements(cursor, statements, parameters, verbosity) django-master/django/db/backends/oracle/creation.py 250 self._execute_statements(cursor, statements, parameters, verbosity) django-master/django/db/backends/oracle/schema.py 57 super().alter_field(model, old_field, new_field, strict) django-master/django/db/backends/oracle/schema.py 68 self.alter_field(model, old_field, new_field, strict) django-master/django/db/backends/mysql/schema.py 97 super()._alter_column_type_sql(model, old_field, new_field, new_type) django-master/django/db/backends/mysql/schema.py 101 super()._rename_field_sql(table, old_field, new_field, new_type) django-master/django/db/backends/base/schema.py 518 self._alter_many_to_many(model, old_field, new_field, strict) django-master/django/db/backends/base/schema.py 532 self._alter_field(model, old_field, new_field, old_type, new_type, old_db_params, new_db_params, strict) django-master/django/db/backends/base/schema.py 626 self._alter_column_type_sql(model, old_field, new_field, new_type) django-master/django/db/backends/base/schema.py 943 self._index_columns(table, columns, col_suffixes, opclasses) django-master/django/db/models/query.py 1041 clone.query.add_extra(select, select_params, where, params, tables, order_by) django-master/django/db/models/expressions.py 239 expr.resolve_expression(query, allow_joins, reuse, summarize) django-master/django/db/models/expressions.py 445 c.lhs.resolve_expression(query, allow_joins, reuse, summarize, for_save) django-master/django/db/models/expressions.py 446 c.rhs.resolve_expression(query, allow_joins, reuse, summarize, for_save) django-master/django/db/models/expressions.py 599 arg.resolve_expression(query, allow_joins, reuse, summarize, for_save) django-master/django/db/models/expressions.py 666 super().resolve_expression(query, allow_joins, reuse, summarize, for_save) django-master/django/db/models/expressions.py 893 c.result.resolve_expression(query, allow_joins, reuse, summarize, for_save) django-master/django/db/models/expressions.py 957 case.resolve_expression(query, allow_joins, reuse, summarize, for_save) django-master/django/db/models/expressions.py 958 c.default.resolve_expression(query, allow_joins, reuse, summarize, for_save) django-master/django/db/models/lookups.py 237 self.resolve_expression_parameter(compiler, connection, sql, param) django-master/django/db/models/aggregates.py 39 super().resolve_expression(query, allow_joins, reuse, summarize) django-master/django/db/models/aggregates.py 40 c.filter.resolve_expression(query, allow_joins, reuse, summarize) django-master/django/db/models/base.py 829 self._do_update(base_qs, using, pk_val, values, update_fields, forced_update) django-master/django/db/models/functions/datetime.py 64 super().resolve_expression(query, allow_joins, reuse, summarize, for_save) django-master/django/db/models/functions/datetime.py 191 super().resolve_expression(query, allow_joins, reuse, summarize, for_save) django-master/django/db/models/sql/compiler.py 652 self.query.join_parent_model(opts, model, start_alias, seen_models) django-master/django/db/models/sql/compiler.py 707 self.find_ordering_name(item, opts, alias, order, already_seen) django-master/django/db/models/sql/query.py 1197 self.resolve_lookup_value(value, can_reuse, allow_joins, simple_col) django-master/django/db/models/sql/query.py 1305 self._add_q( child, used_aliases, branch_negated, current_negated, allow_joins, split_subq) django-master/django/views/decorators/csrf.py 35 super().process_view(request, callback, callback_args, callback_kwargs) / Anders
On Thu, Sep 13, 2018 at 6:50 PM Anders Hovmöller <boxed@killingar.net> wrote:
On 14 Sep 2018, at 03:35, Michael Selik <mike@selik.org> wrote: In that case, you should be able to link to a compelling example. If you go to the trouble of finding one, I'll take time to try to refactor it.
https://github.com/django/django/blob/master/django/db/models/sql/compiler.p...
Is a pretty typical one.
That call is recursive, so it's unlikely that the author would shift the parameters around without testing the call and changing the argument positions appropriately. The signature uses the parameter "default_order" and the call uses the argument "order". It seems that was a deliberate choice that wouldn't conform to the `f(a=a)` pattern. The call is oddly split across lines 707 and 708, despite nearby lines being much longer. it could easily have been written as a single line. I don't find this a compelling example.
In that case, you should be able to link to a compelling example. If you go to the trouble of finding one, I'll take time to try to refactor it.
https://github.com/django/django/blob/master/django/db/models/sql/compiler.p... <https://github.com/django/django/blob/master/django/db/models/sql/compiler.p...>
Is a pretty typical one.
That call is recursive, so it's unlikely that the author would shift the parameters around without testing the call and changing the argument positions appropriately.
Maybe. Still it would be better with keyword arguments. Testing is one thing, quickly finding problems is another. Keyword arguments fail fast and cleanly with signature changes, positional arguments only do when you add or remove parameters at the very end, all other changes are potentially very annoying to debug because you can get a long way past the problem call site before hitting a type or behavior error. I'm not sure we even agree on this basic point though. Do you agree on that?
The signature uses the parameter "default_order" and the call uses the argument "order". It seems that was a deliberate choice that wouldn't conform to the `f(a=a)` pattern.
Maybe. It's a bit weirdly named quite frankly. Do you set the default_order by passing that argument? Or do you set the order? The code below sounds like it's order, but the signature sounds like it's default order. It can't be both, can it? I don't know the specifics but it might just be that this code is hard to read precisely because it doesn't conform to the pattern, and if it *did* use the suggested feature the mismatch in names would stand out and I would believe that it was intentional and not a mistake at the call site, because it would look like: results.extend(self.find_ordering_name( =item, =opts, =alias, default_order=order, =already_seen, )) now the odd ball out stands out instead of hiding. Different things should look different, and similar things should look similar (which would have been a good addition to the Zen of Python imo).
The call is oddly split across lines 707 and 708, despite nearby lines being much longer. it could easily have been written as a single line.
Agreed. It's the ghost of PEP8, but that's a totally different morass! Let's keep to one extremely controversial topic at a time :) / Anders
On Thu, Sep 13, 2018, 11:58 PM Anders Hovmöller <boxed@killingar.net> wrote:
In that case, you should be able to link to a compelling example. If you
go to the trouble of finding one, I'll take time to try to refactor it.
https://github.com/django/django/blob/master/django/db/models/sql/compiler.p...
Is a pretty typical one.
That call is recursive, so it's unlikely that the author would shift the parameters around without testing the call and changing the argument positions appropriately.
Maybe. Still it would be better with keyword arguments. Testing is one thing, quickly finding problems is another. Keyword arguments fail fast and cleanly with signature changes, positional arguments only do when you add or remove parameters at the very end, all other changes are potentially very annoying to debug because you can get a long way past the problem call site before hitting a type or behavior error.
I'm not sure we even agree on this basic point though. Do you agree on that?
I agree those are benefits to keyword arguments, but I disagree that those benefits accrue in all cases. I do not believe that keyword arguments are strictly better than positional. Maybe our difference of opinion stems from tooling and the way others refactor the code we work on. I enjoy using keywords to explain the values I'm passing. If I already have a well-named variable, I'm less keen on using a keyword. Here's a possible refactor. I didn't bother with keyword arguments, because the variable names are easy to match up with arguments positionally. My screen was large enough that I could read the signature at the same time as I wrote the call. def recurse(name): return self.find_ordering_name(name, opts, alias, order, already_seen) return itertools.chain.from_iterable(map(recurse, opts.ordering))
Maybe our difference of opinion stems from tooling and the way others refactor the code we work on.
Maybe. Or the code we have to refactor that others have written. Or both.
I enjoy using keywords to explain the values I'm passing. If I already have a well-named variable, I'm less keen on using a keyword.
Here lies the crux for me. You're describing telling the next reader what the arguments are. But without keyword arguments you aren't telling the computer what the arguments are, not really. The position and the names become disconnected when they should be connected. It's very similar to how in C you use {} for the computer but indent for the human, and no one checks that they are the same (not true anymore strictly because I believe clang has an option you can turn off to validate this but it's default off). In python you tell the computer and the human the same thing with the same language. This is robust and clear. I think this situation is the same.
Here's a possible refactor. I didn't bother with keyword arguments, because the variable names are easy to match up with arguments positionally. My screen was large enough that I could read the signature at the same time as I wrote the call.
def recurse(name): return self.find_ordering_name(name, opts, alias, order, already_seen) return itertools.chain.from_iterable(map(recurse, opts.ordering))
I don't see how that changed anything. That just changes it to a functional style, but otherwise it's identical. / Anders
participants (5)
-
Anders Hovmöller -
Chris Angelico -
Jonathan Fine -
Michael Selik -
Terry Reedy