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

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

Merged

Conversation

@lafrech
Copy link
Member

commented Nov 28, 2017

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

This comment has been minimized.

Copy link
Member

commented Nov 28, 2017

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

This comment has been minimized.

Copy link
Member

commented Jan 31, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2018

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

This comment has been minimized.

Copy link
Member

commented Jan 31, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2018

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 comment has been minimized.

Copy link
Member

commented Feb 2, 2018

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

This comment has been minimized.

Copy link
Member

commented Feb 2, 2018

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.

@sloria sloria merged commit 78d2153 into marshmallow-code:dev Feb 2, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@lafrech lafrech deleted the Nobatek:dev_flask_abort_always_add_data branch Sep 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.