-
-
Notifications
You must be signed in to change notification settings - Fork 385
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added support for keyword-only arguments on Python 3+ #281
Conversation
@hynek I decided to go with a dedicated |
Wow, thanks for tackling this! Before I dive deeper and start nitpicking:
|
Codecov Report
@@ Coverage Diff @@
## master #281 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 9 9
Lines 660 679 +19
Branches 138 144 +6
=====================================
+ Hits 660 679 +19
Continue to review full report at Codecov.
|
Done.
Or we could make Python 3 transition slightly faster :) With no jokes, although I'm against of supporting such unique Python 3 features on Python 2 I'll try to implement this fallback anyway. |
Ohhh…so you implemented it only on a per-attribute basis instead of per-class? |
(JFTR I have zero qualms leaving Python 2 behind in 2017 if the cost exceeds a certain threshold) |
Yes. In my opinion, if we have a per-attribute way of specifying keyword-only attributes making a whole class keyword-only is easy: from functools import partial
kw_attrib = partial(attr.ib, kw_only=True)
@attr.s
class Foo:
bar = kw_attrib()
baz = kw_attrib() But maybe an additional |
dd7e95b
to
57c5b0e
Compare
@hynek regarding Python 2 emulation of keyword-only arguments, I've found https://github.com/pasztorpisti/kwonly-args. Python 2.7.14 (default, Oct 8 2017, 18:34:38)
[GCC 4.2.1 Compatible Apple LLVM 8.1.0 (clang-802.0.38)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from kwonly_args import first_kwonly_arg, KWONLY_REQUIRED
>>> @first_kwonly_arg('b')
... def foo(a, b=KWONLY_REQUIRED, c=KWONLY_REQUIRED, d=4):
... pass
...
>>> foo()
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/Users/malinoff/Projects/opensource/attrs/venv27/lib/python2.7/site-packages/kwonly_args/__init__.py", line 108, in wrapper
', '.join(sorted(missing_kwonly_args))))
TypeError: foo() missing 2 keyword-only argument(s): b, c
>>> foo(1, 2, 3)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/Users/malinoff/Projects/opensource/attrs/venv27/lib/python2.7/site-packages/kwonly_args/__init__.py", line 108, in wrapper
', '.join(sorted(missing_kwonly_args))))
TypeError: foo() missing 2 keyword-only argument(s): b, c vs Python 3.6.1 (default, May 29 2017, 09:11:03)
[GCC 4.2.1 Compatible Apple LLVM 8.1.0 (clang-802.0.38)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> def foo(a, *, b, c, d=4):
... pass
...
>>> foo()
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: foo() missing 1 required positional argument: 'a'
>>> foo(1, 2, 3)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: foo() takes 1 positional argument but 3 were given And the implementation is scary enough for me not wanting to translate it into the list of lines used to generate the What do you think about adding this as a Python 3 only feature for now? |
Rebased onto master. |
@Tinche since you've been pretty active in the referenced issues, I'd like to hear your opinion too. |
As I’ve said, I don’t mind leaving Python 2 behind if it means too much work. @Tinche is currently having a life-or-death battle with a manflu, but this won’t make 17.3 anyways, so no pressure. :) |
@malinoff Implementing the additional Quoting an usage example from the docs
|
@leezu having only an Also, I've opened this PR before 17.3 have been released, so I couldn't take @hynek @Tinche, since 17.3 was cut off, could you please take a closer took at this PR? |
would it be acceptable for python2 support to use a (given that popping from the dict + asserting can work, but the signature would suck |
@malinoff essentially I would like to avoid writing out
Do you mean the You can argue that using keyword-only arguments is an advanced use-case, so it should require the "more complex"
What about not "only an attr.s option" but an "shortcut attr.s option" which defaults to |
That's what I mean in my previous reply. |
So to come back to this: IIRC this would be the first option that exists only on the attributes but not for the whole class? I think we should really allow both: in attr.ib to None (=default) and add an option to attr.s. If you don’t want to do it here, it’s fine: we can finish this one up but block 17.4. until the second part is done. At this moment, this has to be rebased. |
Sorry for bursting into your conversation, but I hope it's related enough.
I guess it's more related to your plan to add an option to attr.s and not the attr.ib one. |
I see this is not implemented in the code yet, will it be a part of version 17.4? |
Unlikely, 17.4 is gonna be a mostly (overdue) bug fix release that should drop next week if nothing comes up. |
@malinoff Is there anything else that needs to happen with this PR? I'm happy to take over the branch to try to get it in if you don't have the time). (Although for my own use-case, I just need the class-level |
See also python/cpython#6238 |
I'm okay to finish the PR by myself, I just don't understand what else should be implemented/fixed. |
Sorry to be a bother, but bump @hynek about what more needs to happen here. |
I’m not gonna review the concrete code now, here’s the general requirements for this to be merged:
So it should work like this: @attr.s(kw_only=True)
class C:
x = attr.ib()
y = attr.ib() -> @attr.s
class C:
x = attr.ib()
y = attr.ib(kw_only=True) -> I think it’s fair to expect that a non-kw_only attribute must not come after a kw_only attribute. Am I miss something? I’m sorry for not being very responsive. (Please disregard my comments from before about setting attr.ib's kw_only to None – that would be inconsistent with the rest of attrs. It should be False of course.) |
closes python-attrs#38 (Rebases python-attrs#281) Co-authored-by: Alex Ford <fordas@uw.edu>
* Added support for keyword-only attributes. Closes #106, and closes #38 (Rebases #281) Co-authored-by: Alex Ford <fordas@uw.edu> * Add `attr.s`-level `kw_only` flag. Add `kw_only` flag to `attr.s` decorator, indicating that all class attributes should be keyword-only in __init__. Minor updates to internal interface of `Attribute` to support evolution of attributes to `kw_only` in class factory. Expand examples with `attr.s` level kw_only. * Add `kw_only` to type stubs. * Update changelog for rebased PR. Hear ye, hear ye. A duplicate PR is born. * Tidy docs from review. * Tidy code from review. * Add explicit tests of PY2 kw_only SyntaxError behavior. * Add `PythonToOldError`, raise for kw_only on PY2. * `Attribute._evolve` to `Attribute._assoc`.
fixed by #411 |
Pull Request Check List
.rst
files is written using semantic newlines.versionadded
,versionchanged
, ordeprecated
directives.changelog.d
.Please, correct me if I checked something wrong.