flaskparser.abort: add data to exception even if kwargs is empty

[lafrech]

Any objection to this?

It doesn't break any existing test.

The idea is to avoid checking that exception has data attribute and get its content directly.

Currently, I need to do

    # Use one kwarg
    if hasattr(error, data):
        error.data.get(my_kwarg, default_val)

    # Use all kwargs
    if hasattr(error, data):
        my_function(**error.data)

With this change, I would just do

    # Use one kwarg
    error.data.get(my_kwarg, default_val)

    # Use all kwargs
    my_function(**error.data)

And if I just want to check that args were passed, I'll do

    if error.data:

rather than

    if hasattr(error, data):

[sloria]

Thanks for the PR @lafrech . I think this is the right thing to do, but the abort code was taken from Flask-RESTful a long time ago.

Before merging this, I'd like to hear from the Flask-RESTful maintainer(s) if there was good reason to conditionally add .data. I posted an issue : flask-restful/flask-restful#723 . Let's follow discussion there.

[sloria]

I think we should move forward with this. Even though this behavior isn't explicitly tested, I do think it's a breaking change.

Perhaps we'll roll this and your Marshmallow 3 PR into a webargs 2.0 release =).

[lafrech]

Alright.

Note that the MA3 PR is supposed to keep MA2 compatibility, so it does not involve a major release.

If you wish to drop MA2 compatibility in a webargs 2 release, that's fine with me. The MA2 specific parts should be easy to spot and remove.

[sloria]

Ah, that's right--forgot that the MA3 PR is MA2-compatible.

I'm torn about keeping MA2-compatibility in webargs 3. I think if we do keep MA2-compat, we wouldn't need to bother supporting two release lines in webargs.

Do you have a preference either way, @lafrech ?

[lafrech]

I answered about MA2/MA3 in #188.

Regarding this specific change, I don't really mind. I mean, it is pretty minor. I hit this once, and after sending this PR, I realized I had to check anyway because the HTTPException could be raised using Flask vanilla abort (https://github.com/Nobatek/flask-rest-api/blob/master/flask_rest_api/error_handler.py#L50). In other words, merging this would have no effect on my code.

Maybe you can tag this webargs 2 and leave it open until there are breaking changes worth a major release.

I can't really figure out a use case that would be broken by merging this, though. If no kwargs are passed, someone will get an empty dict instead of nothing. I'm not convinced it is a "beaking" change. But again, I really don't mind at all. It is not worth wasting too much time. Do whatever is simpler for you.

[sloria]

This could break code that relied on hasattr(exc, 'data'), but maybe it isn't a big deal; like you said, they'll just get an empty dict, which is falsy.

Honestly, I don't feel too strongly either way. But whenever I have any doubt about whether something's a breaking change or not, I try to err on the side of caution.

[sloria]

On the other hand, I don't think this warrants a major version bump. It's unlikely to affect most users, and I'd rather not maintain another release line for such a minute change, so consider my mind changed. Let's get this merged.