Minor refactors and pylint fixes

[lafrech]

WIP

[lafrech]

I'd like to finish this before releasing 6.0.0.

There is a few other things I'd like to change.

And there's stuff I'd like to double-check.

• I'm not sure about the need for or {} in kwargs.update(parsed_args or {}). I made things consistent but we perhaps could remove it everywhere.
• I tried to harmonize the use_kwargs / use_args branch in all parsers, but this may change the order in which the parameters are passed in pyramid.

[lafrech]

I'm not sure about the need for or {} in kwargs.update(parsed_args or {}).

Unneeded because _load_location_data returns an empty dict so I see no case where load would return None instead of an empty dict.

I tried to harmonize the use_kwargs / use_args branch in all parsers, but this may change the order in which the parameters are passed in pyramid.

I confirm it does. But towards an order consistent with the other parsers, so let's stick with this and do it in a major update.

[lafrech]

I'd like to merge this and release 6.0.0.

Extension of typing to all the code can be done later.

[sirosen]

I agree with you about making pyramid consistent with the other parsers. I think we should add a changelog note about this though, since it's likely to otherwise be confusing.

I've reviewed all of the changes and it all looks good. Will wait for your answer on the changelog before merging though.

[lafrech]

Sure, it deserves a CHANGELOG entry. I can include it in the PR before merging.

BTW, I was convinced the pyramid parser could be refactored to avoid duplicating the core parser. Basically, the idea was that the arguments may vary but we could move the try/except logic into a get_request_from_view_args method. I spent quite some time trying to do that but I'm stuck on something I don't understand. The refactor involves removing obj argument to merge it with *args except when I do that I don't get the same arguments fed to the view function. There's definitely something I'm missing. I asked on SO: https://stackoverflow.com/q/60418143/4653485.

[sirosen]

I've been digging through Pyramid to try to figure out how it's making the decision (it must use inspect somewhere or similar). It seems that view functions in Pyramid can have one of two signatures: https://docs.pylonsproject.org/projects/pyramid/en/latest/glossary.html#term-view-callable

I also see that they generate a view func with that signature if none is provided and you're using a renderer instead:
https://github.com/Pylons/pyramid/blob/a71df99b57e88788cf9ce3a78fc005f309033bbd/src/pyramid/config/views.py#L809-L813

So somehow the pyramid view inspection is broken by the layer of decoration, and the (context, view) convention is the default. But if there's a single named positional argument, it works correctly.

I'm still trying to see where this is deduced, in the hope that it makes it possible to somehow tell pyramid what we want.

[sirosen]

Okay! I found where inspect is used and why/how this all shakes out.
Because you have to traverse a lot of the framework to get here, I won't try to follow everything, but the path of wrappers/decorators/etc leads you to a phase in which Pyramid decorates the user's callable.

One of those steps checks if it's a requestonly callable to wrap as a (context, request) callable.
requestonly is just an alias for takes_one_arg, which is where the most relevant part happens. Mostly, it's looking at inspect.getfullargspec().args.

Popping open an interpreter really quick:

>>> import inspect
>>> def foo(*args, **kwargs):
...     pass
...
>>> inspect.getfullargspec(foo)
FullArgSpec(args=[], varargs='args', varkw='kwargs', defaults=None, kwonlyargs=[], kwonlydefaults=None, annotations={})

And getfullargspec is not going to traverse a decorator, even with functools.wraps, since decorators could change the arguments passed.

---

I don't know if we want to try to unify the pyramid parser with the core one in this branch.

The thing which occurs to me is that we could -- because we know about pyramid's behavior -- do this:

def takes_one_arg(func):
    return len(inspect.getfullargspec(func).args) == 1


def parse_args(...):
    ...
    if takes_one_arg(func):
        @functools.wraps(func)
        def wrapped(request, *args, **kwargs):
            ...
    else
        @functools.wraps(func)
        def wrapped(*args, **kwargs):
            ...
    return wrapped

but that seems worse to me than the current way of supporting it (with a specialized version of use_args).

[lafrech]

Yeah, we can't do any better, I'm afraid. No big deal. Thanks for your help on this.

I guess we can merge this and release, then.

[sirosen]

@lafrech, just so we're on the same page, this is merged but I'm not tagging/releasing things until/unless we have a conversation about it to confirm that I should.

(I assume release procedure is just the same as the marshmallow one?)

[lafrech]

just so we're on the same page, this is merged but I'm not tagging/releasing things until/unless we have a conversation about it to confirm that I should.

This is the right thing to do. I see nothing else to add so we can release.

I assume @sloria would have told us if he wanted to postpone the release.

(I assume release procedure is just the same as the marshmallow one?)

Exactly. I always refer to this file.

You may do it or I'll do it when I get the time.

Great !

[sloria]

🚢 🇮🇹