Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for keyword-only arguments on Python 3+ [rebase] #411

Merged
merged 14 commits into from Aug 11, 2018

Conversation

@asford
Copy link
Contributor

@asford asford commented Jul 24, 2018

This is a rebase-and-extension of pull #281. @malinoff (& @hynek), I'm happy to handle this as a sub-pull of that request or merge independently.

Closes #335, closes #106, closes #38.

Pull Request Check List

This is just a reminder about the most common mistakes. Please make sure that you tick all appropriate boxes. But please read our contribution guide at least once, it will save you unnecessary review cycles!

  • Added tests for changed code.
  • New features have been added to our Hypothesis testing strategy.
  • Changes or additions to public APIs are reflected in our type stubs (files ending in .pyi).
  • Updated documentation for changed code.
  • Documentation in .rst files is written using semantic newlines.
  • Changed/added classes/methods/functions have appropriate versionadded, versionchanged, or deprecated directives.
  • Changes (and possible deprecations) have news fragments in changelog.d.

If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!

(Rebases #281)

Co-authored-by: Alex Ford <fordas@uw.edu>
@asford asford changed the title Issue 106 Added support for keyword-only arguments on Python 3+ [rebase] Jul 24, 2018
@asford
Copy link
Contributor Author

@asford asford commented Jul 24, 2018

@hynek I think this extends #281 to resolve the feedback you provided in that review. Would you mind taking a look at this implementation?

@hynek
Copy link
Member

@hynek hynek commented Jul 24, 2018

Thanks for reviving this! I’ll be at EuroPython for the rest of the week but will take a look once I have the time. You’ve Got a typo in the stubs btw: kw_onlly or something like that (on phone sorry).

Other reviewers are invited to pick up my slack of course. ;)

@asford
Copy link
Contributor Author

@asford asford commented Jul 24, 2018

Great, thanks! Before I start covering my tracks, is --force push ok for fixups in PRs in this repo?

@hynek
Copy link
Member

@hynek hynek commented Jul 24, 2018

It’s your repo, do whatever the hell pleases you. We’ll squash at the end anyway.

asford added 2 commits Jul 24, 2018
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.
@asford asford force-pushed the uw-ipd:issue-106 branch from 0d488a9 to 69ad3aa Jul 24, 2018
Hear ye, hear ye. A duplicate PR is born.
@asford
Copy link
Contributor Author

@asford asford commented Jul 24, 2018

Fixed .pyi typo and delinted; ready for review at everyone's convenience.

Copy link
Member

@hynek hynek left a comment

Overall this looks pretty good! I have a bunch of minor things and one major question. :)

>>> A(a=1)
A(a=1)
`kw_only` may also be specified at via ``attr.s``, and will apply to all attributes:

This comment has been minimized.

@hynek

hynek Jul 28, 2018
Member

kw_only needs double ticks



If you create an attribute with ``init=False``, ``kw_only`` argument is simply ignored.

This comment has been minimized.

@hynek

hynek Jul 28, 2018
Member

… *the* ``kw_only`` argument...

and please scratch the “simply”.

...
TypeError: B() missing 1 required keyword-only argument: 'b'
If you omit ``kw_only`` or specify ``kw_only=False``, then you'll get an error:

This comment has been minimized.

@hynek

hynek Jul 28, 2018
Member

I think can be simplified to “If you don’t set kw_only=True, then …”

This comment has been minimized.

@asford

asford Jul 28, 2018
Author Contributor

Added a slight explanation for the generated error.

@@ -206,6 +212,7 @@ def attrib(
converter=converter,
metadata=metadata,
type=type,
kw_only=kw_only,

This comment has been minimized.

@hynek

hynek Jul 28, 2018
Member

Since this is Python 3 only, maybe we should raise an error if someone tries to use it in Python 2? Otherwise I already see the bug reports that kw_only “doesn’t work”.

This comment has been minimized.

@asford

asford Jul 28, 2018
Author Contributor

🤔 The current implementation raises SyntaxError from the generated __init__ during class decoration. Would you prefer to raise a different exception with an explicit error message?

This comment has been minimized.

@asford

asford Jul 28, 2018
Author Contributor

I've added an explicit test of the SyntaxError behavior in 42ec86d to help clarify the discussion.

This comment has been minimized.

@hynek

hynek Jul 31, 2018
Member

Yeah I think I’d prefer an explicit error for that. I think it’s worth it to introduce a PythonTooOldError("Keyword-only arguments only work on Python 3 and later.", (3, 4)) since something like this will come up more often I presume?

This comment has been minimized.

@asford

asford Jul 31, 2018
Author Contributor

It's hard to imagine a case in which calling code would catch the generated exception, so this is really a question over which error message is propagated up to the user when the system exits. I'll happily defer to your opinion, but there are a few arguments that I'd like to make explicit before adding the exception class:

  1. This feature throws a relatively clear exception on python 2, and the attrs documentation is rather explicit about the feature being python 3+ only. From that perspective, RTFD and let's keep extra version-specific error logic out of the library.

...however...

  1. attrs is low-level dependency for many tools. Many errors will be the result of an indirect dependency on attrs though some other intermediate library. Expecting potentially-less-savvy users to to read and comprehend a SyntaxError trace-back, then connect this to an intermediate use of the kw_only feature is completely unreasonable overly idealistic.

This comment has been minimized.

@asford

asford Jul 31, 2018
Author Contributor

I don't have a solid intuition for where this error check should occur, though my instinct is that we should find a single place to insert the check. Would you be ok with throwing from within _attrs_to_init_script iff keyword-only args are being specified?

@@ -736,6 +760,10 @@ def attrs(
Attributes annotated as :data:`typing.ClassVar` are **ignored**.
.. _`PEP 526`: https://www.python.org/dev/peps/pep-0526/
:param bool kw_only: Make all attributes keyword-only (Python 3+)
in the generated ``__init__`` (if ``init`` is ``False``, this
parameter is simply ignored).

This comment has been minimized.

@hynek

hynek Jul 28, 2018
Member

Please scratch the “simply”

x = attr.ib(default=None)
y = attr.ib()

attrs, super_attrs, _ = _transform_attrs(C, None, False, True)

This comment has been minimized.

@hynek

hynek Jul 28, 2018
Member

empty line before asserts please

x = attr.ib(init=False, default=0, kw_only=True)
y = attr.ib()

c = C(1)

This comment has been minimized.

@hynek

hynek Jul 28, 2018
Member

empty line before asserts please

y = attr.ib()

with pytest.raises(ValueError) as e:
_transform_attrs(C, None, False, False)

This comment has been minimized.

@hynek

hynek Jul 28, 2018
Member

empty line before asserts please

with pytest.raises(TypeError):
C(0, y=1)

c = C(x=0, y=1)

This comment has been minimized.

@hynek

hynek Jul 28, 2018
Member

empty line before asserts please

with pytest.raises(TypeError):
C(1)

c = C(x=0, y=1)

This comment has been minimized.

@hynek

hynek Jul 28, 2018
Member

empty line before asserts please

@hynek hynek added this to the 18.2 milestone Jul 28, 2018
@asford asford force-pushed the uw-ipd:issue-106 branch from ce20d8e to 57b404f Jul 28, 2018
@asford asford force-pushed the uw-ipd:issue-106 branch from 57b404f to 42ec86d Jul 28, 2018
Copy link
Member

@hynek hynek left a comment

We’re getting there!

@@ -1558,6 +1613,17 @@ def from_counting_attr(cls, name, ca, type=None):
**inst_dict
)

# Don't use attr.evolve since fields(Attribute) doesn't work
def _evolve(self, **changes):

This comment has been minimized.

@hynek

hynek Aug 1, 2018
Member

This might seem pedantic, but what you do here doesn't mirror our evolve but our assoc. Just call it what it does: copy_then_set or something.

This comment has been minimized.

@asford

asford Aug 1, 2018
Author Contributor

Renamed to match the semantics. As Attribute isn't an attrs class assoc can't be used for the same reason, it depends on fields. I'd avoided the assoc name as the function appears to be deprecated and, in this class, the functions are effectively equivalent.

if kw_only_args:
if PY2:
raise PythonTooOldError(
"Keyword-only arguments only work on Python 3 and later."

This comment has been minimized.

@hynek

hynek Aug 1, 2018
Member

Let’s add a helpful helpful information on the minimal version:

@attr.s
class MinimumPythonVersion(object):
    major = attr.ib()
    minor = attr.ib()
    micro = attr.ib(default=0)

This comment has been minimized.

@asford

asford Aug 1, 2018
Author Contributor

Can you comment on where you'd prefer to see this class should be defined? I don't think you've any instances of attrs-using-attrs at this level of the library.

  1. This class, if defined via attrs, can't be added in exceptions without introducing a cyclic dependency on _make. Defining within exceptions would require indirection of all exception access in _make (eg. exceptions.PythonTooOld rather than from exceptions import PythonTooOld) to break the import-time cyclic dependency.

  2. We could add a "higher-level' exceptions module that depends on _make including just this exception and attrs class, but this begins adding an ambiguous forest of modules.

  3. We could define it in _make, but @attrs won't be available until relatively late in in the module and it feels wrong to have the exception and it's "necissary data" defined in different locations. This also couples any further use of the exception to _make.

My preference is for (1).

This comment has been minimized.

@hynek

hynek Aug 1, 2018
Member

Ugh yeah you’re right. We can add this later; leave it be for now.

This comment has been minimized.

@asford

asford Aug 1, 2018
Author Contributor

Got it. We'll also defer the version checks you indicated in the tests then?


class PythonTooOldError(RuntimeError):
"""
An ``attrs`` feature requiring a more recent python version has been used.

This comment has been minimized.

@hynek

hynek Aug 1, 2018
Member

It should be mentioned the args are msg, MinimumPythonVersion

@attr.s(kw_only=True)
class ClassLevel(object):
a = attr.ib()

This comment has been minimized.

@hynek

hynek Aug 1, 2018
Member

this will have to test for MinimumPythonVersion

@attr.s()
class AttrLevel(object):
a = attr.ib(kw_only=True)

This comment has been minimized.

@hynek

hynek Aug 1, 2018
Member

same

@hynek
hynek approved these changes Aug 11, 2018
@hynek hynek merged commit 123df67 into python-attrs:master Aug 11, 2018
3 checks passed
3 checks passed
@codecov
codecov/patch 100% of diff hit (target 100%)
Details
@codecov
codecov/project 100% (target 100%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hynek
Copy link
Member

@hynek hynek commented Aug 11, 2018

Thanks everyone!

@hynek hynek mentioned this pull request Aug 11, 2018
6 of 6 tasks complete
@wbolster
Copy link
Member

@wbolster wbolster commented Aug 13, 2018

this is awesome! thanks @asford for pushing this through!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants