Skip to content

Conversation

jcass77
Copy link
Contributor

@jcass77 jcass77 commented Feb 7, 2017

As far as I could tell, these are the only changes required to get the test back to green.

None of the CSRF-related changes appear to affect flask-mongoengine (so far).

@lafrech lafrech merged commit 96b81bd into MongoEngine:master Feb 7, 2017
@lafrech
Copy link
Member

lafrech commented Feb 7, 2017

Thanks @jcass77 for the contribution.

def __init__(self, formdata=None, obj=None, **kwargs):
self.instance = (kwargs.pop('instance', None) or obj)
def __init__(self, formdata=None, **kwargs):
self.instance = (kwargs.pop('instance', None) or kwargs.get('obj'))
Copy link
Contributor Author

@jcass77 jcass77 Feb 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...thinking about this some more, this should probably be kwargs.get('obj', None) to avoid a KeyError being raised seeing as this is an optional parameter.

Do I need to submit a new PR or will somebody with commit access just make the change?

Copy link
Member

@lafrech lafrech Feb 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm... dict.get defaults to None, right?

assert {'a':1, 'b':2}.get('d') is None

(This is inconsistent with getattr and I was already struck by it once recently.)

Thanks anyway for taking the time to review your own code again. I appreciate.

And considering the little time we dedicate to the maintenance of flask-mongoengine, a PR is appreciated, even for a trivial change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, without checking, I assumed that the code would produce the same result as {'a':1, 'b':2}['d'] - my bad.

@wojcikstefan
Copy link
Member

Thank you for taking care of this @jcass77 and @lafrech!

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

Successfully merging this pull request may close these issues.

3 participants