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

Don't extract decorators with names matching the 'data_key' of defined schema fields #85

Merged
merged 5 commits into from
Jul 22, 2019

Conversation

andrewwhitehead
Copy link
Contributor

@sklump Re: Aries#36 support

@andrewwhitehead andrewwhitehead requested a review from nrempel July 17, 2019 22:59
@codecov-io
Copy link

codecov-io commented Jul 17, 2019

Codecov Report

Merging #85 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #85      +/-   ##
==========================================
+ Coverage   70.98%   71.02%   +0.04%     
==========================================
  Files         177      177              
  Lines        7524     7535      +11     
==========================================
+ Hits         5341     5352      +11     
  Misses       2183     2183

@sklump
Copy link
Contributor

sklump commented Jul 18, 2019

Unless I've misread the intent, this approach still needs about 20 lines of boilerplate per descendant to replace one parameter, skip_attrs, in AgentMessageSchema.extract_decorators() call to self._decorators.extract_decorators().

Could taking skip_attrs into extract_decorators itself, like this:

@pre_load
def extract_decorators(self, data, skip_attrs: Sequence[str] = None):
    """..."""
    processed = self._decorators.extract_decorators(data, self.__class__, skip_attrs if skip_attrs else None)
    ...
    return processed

then in descendant

@pre_load
def extract_decorators(self, data):
    return super().extract_decorators(data, ["skip", "these", "attrs"])

work OK? Apart from the one line delta, I think the rest of the processing ought to be identical in descendants.

nrempel
nrempel previously approved these changes Jul 18, 2019
Copy link
Contributor

@nrempel nrempel left a comment

Choose a reason for hiding this comment

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

Everything looks good to me

@sklump
Copy link
Contributor

sklump commented Jul 18, 2019

I will also propagate this change into my Aries#0036 PR by tomorrow morning.

@nrempel
Copy link
Contributor

nrempel commented Jul 18, 2019

Unless I've misread the intent, this approach still needs about 20 lines of boilerplate per descendant to replace one parameter, skip_attrs, in AgentMessageSchema.extract_decorators() call to self._decorators.extract_decorators().

Could taking skip_attrs into extract_decorators itself, like this:

@pre_load
def extract_decorators(self, data, skip_attrs: Sequence[str] = None):
    """..."""
    processed = self._decorators.extract_decorators(data, self.__class__, skip_attrs if skip_attrs else None)
    ...
    return processed

then in descendant

@pre_load
def extract_decorators(self, data):
    return super().extract_decorators(data, ["skip", "these", "attrs"])

work OK? Apart from the one line delta, I think the rest of the processing ought to be identical in descendants.

@sklump I didn't mean to clobber your comment -

I do like the idea of keeping things declarative. @andrewwhitehead right now, the intent is that the sub-class of agent_message would override @pre_load and pass in skip_attrs?

What about something declarative like Stephen's approach, but checking skip_attrs in a Meta class variable?

@andrewwhitehead
Copy link
Contributor Author

It's not meant to be necessary to override extract_decorators, I just added skip_attrs as an optional parameter there in case somebody's using it on non-Schema'd content. It should be sufficient to declare fields like the one in the test case: handled_decorator = fields.Str(required=False, data_key="handled~decorator") and have them automatically skipped when the decorators are loaded, to be parsed by the Schema itself.

@sklump
Copy link
Contributor

sklump commented Jul 18, 2019

OK, I will try to get it to work and adapt accordingly by tomorrow morning.

Signed-off-by: Andrew Whitehead <[email protected]>
@swcurran swcurran self-requested a review July 22, 2019 17:14
Copy link
Contributor

@swcurran swcurran left a comment

Choose a reason for hiding this comment

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

I've heard good things about this from @ianco and looks OK to me.

@swcurran swcurran merged commit 32e5a92 into openwallet-foundation:master Jul 22, 2019
@andrewwhitehead andrewwhitehead deleted the skip-decorator branch February 29, 2020 21:45
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.

6 participants