Use kw-only arguments where sensible

[lafrech]

Closes the second item in #440.

[lafrech]

Looks like this is not allowed in Python 3.5

    def parse(
        self,
        argmap,
        req=None,
        *,
        location=None,
        validate=None,
        error_status_code=None,
        error_headers=None,
    ):

I can't find a documented change in 3.6 explaining this.

I could go with

    def parse(
        self,
        argmap,
        *,
        req=None,
        location=None,
        validate=None,
        error_status_code=None,
        error_headers=None,
    ):

but wouldn't it be too much?

[sirosen]

Just in case someone else thinks of it, I tried the "too clever solution" of making parse's signature different in different pythons, but it doesn't work easily because py3.5 can't even parse the desired signature for 3.6+. I'll share because it's fun to think about:

polyglot_parse decorator

# decorate it to have different signatures
# doesn't work in py3.5
def polyglot_parse(real_parse_func):
    if sys.version_info < (3, 6):
        def parse(
            self,
            argmap,
            req=None,
            location=None,
            validate=None,
            error_status_code=None,
            error_headers=None,
        ):
            return real_parse_func(
                self,
                argmap,
                req=req,
                location=location,
                validate=validate,
                error_status_code=error_status_code,
                error_headers=error_headers,
            )
    else:
        def parse(
            self,
            argmap,
            req=None,
            *,
            location=None,
            validate=None,
            error_status_code=None,
            error_headers=None,
        ):
            return real_parse_func(
                self,
                argmap,
                req=req,
                location=location,
                validate=validate,
                error_status_code=error_status_code,
                error_headers=error_headers,
            )

    return parse

It is possible if we're determined -- you can do things like adding a second module which only gets imported in 3.6, or write it all as a string and use exec! But... let's not go there, right? It seems like a mess.

There's also the "faking it" way with a *args list:

def parse(
    self,
    argmap,
    *args,
    req=None,
    location=None,
    validate=None,
    error_status_code=None,
    error_headers=None,
):
    if len(args) > 1:
        raise ValueError
    if args and req is not None:
        raise ValueError
    if args[0] is not None and req is None:
        req = args[0]
    ...

But this also seems awkward and undesirable.

Python 3.5 EOLs later this year, but I don't want to wait that long to be able to use webargs 6.

I'm not sure if there's a potential issue is with req being keyword-only, other than it not being the API we want for parse (I agree with the proposed, ideal function signature, with req as an optional positional arg). Should we just do that, even though it's not perfect?

The major alternative I see is just leaving this as-is, on the grounds that we can't do what we really want. It can be changed in a later major version upgrade.

[lafrech]

Thanks for looking into this. I don't want to overthink it. I just exposed it here in the hope to get an explanation because I couldn't find any item in the changelog explaining this.

I agree we don't want to do anything messy.

Either we decide req is keyword only. It means modifying a lot of calls in the tests. I don't mind doing this but I figured it might be less practical for client code calling parse. In practice the calls are probably centralized in decorators, not scattered among the whole client application code, so it's not that bad. Also, it is not coherent with the way req is treated in handle_error. But it is not that bad either.

Or we just skip this method.

No big deal either way. I'll settle this one way or another.

[lafrech]

Ah ah. Turns out the issue was not exactly the kw-only thing but the trailing comma in the signature.

Apparently, the trailing comma is fine, unless there is a kw-only * in the signature.

So this works:

    def parse(
        self,
        argmap,
        req=None,
        *,                  # <-----------
        location=None,
        validate=None,
        error_status_code=None,
        error_headers=None
    ):
        pass

    def parse(
        self,
        argmap,
        req=None,
        location=None,
        validate=None,
        error_status_code=None,
        error_headers=None,                  # <-----------
    ):
        pass

But this does not:

    def parse(
        self,
        argmap,
        req=None,
        *,                  # <-----------
        location=None,
        validate=None,
        error_status_code=None,
        error_headers=None,                  # <-----------
    ):
        pass

Go figure...

(I just found an online Python 3.4 interpreter to test that.)

[sirosen]

Yeah, I just checked and that works on my py35 version (3.5.6). And I upgraded to 3.5.9 to check, works on that too.
So I think it's safe to say that 3.5 in general supports this.

Weird though. shrugs

[sirosen]

Should schema be a required keyword-only arg?
I would have left all required arguments as positionals and made only the optional arguments (error_status_code, error_headers) keyword-only.
I also see some variation in the signatures for this in the various parsers, which I assume is accidental.

This is not critical, since handle_error is only really called internally and exposed as a hook for users to write something which gets called by webargs (if they write their own parser subclassing the core parser). But the two signatures which occur to me are

# everything keyword-only
def handle_error(
    self, *, error, req, schema, error_status_code=None, error_headers=None
):
# optional args keyword-only
def handle_error(
    self, error, req, schema, *, error_status_code=None, error_headers=None
):

[lafrech]

Thanks for reviewing. I pushed a new version.

I didn't find the signature variations but anyway, I checked everywhere and I think it is fine, now. I also changed the signature in docstrings/docs.

[sirosen]

I didn't find the signature variations but anyway, I checked everywhere and I think it is fine, now. I also changed the signature in docstrings/docs.

The thing which I see varying is whether or not error_status_code, error_headers are given default values of None.
Almost everywhere, I see these without defaults, but the core parser version and some of the testsuite versions use error_status_code=None, error_headers=None.

I don't think this really has any impact -- those values should always be supplied when an error handler is called -- but now that I've noticed it, I can't help but suggest making them all identical.

(If I'm reading correctly, this was actually slightly inconsistent before, with some locations declaring them as positional arguments and some as keyword arguments.)

[lafrech]

Good catch

I reworked the PR to make these consistent.

error_status_code and error_headers now default to None in parse and use_args and are required in error handler (which as I agree does not make much difference since we know they are supplied).

[sirosen]

This looks good to me. I don't know if we need @sloria to weigh in on this or not.
(I am, after all, still just some guy on the Internet, not a maintainer. 😄 )

[lafrech]

I'd like @sloria to approve the 6.0.0 release, be it before or after we merge this.

I am, after all, still just some guy on the Internet, not a maintainer.

Good point. I just sent you an invitation to join the maintainers team.

[sloria]

I'll take a look at the release when I have some time, but don't block on me! I consider @lafrech more the owner of webargs at this point. Also glad to see @sirosen as maintainer =).

As discussed on Slack, my only hesitation here is req as a kw-only argument. That should probably be positional, but I have low confidence in that opinion.

P.S. @sirosen email me at sloria1 AT gmail.com if you'd like to join the marshmallow slack.

[lafrech]

I agree. I just edited the PR to make req positional. Thanks for your feedback on this.

[sirosen]

I keep hesitating to merge this because the last couple of changes have been so small and subtle, but I think this is good now and I'm going to go ahead. Using my newfound powers!

I think this takes us down to "last call" for changes in 6.0.

[lafrech]

I think this takes us down to "last call" for changes in 6.0.

Yep. Let's release a last beta. Then it leaves us a few days to update the docs (upgrading instructions) during which clients can provide feedback about the beta.

[lafrech]

I was about to write an upgrading guide, but on second thought, the comprehensive instructions in the CHANGELOG should be enough. I didn't see the point of duplicating them in another doc page.