Skip to content

Make field2property extensible#478

Merged
lafrech merged 7 commits intodevfrom
extensible-field2property
Aug 31, 2019
Merged

Make field2property extensible#478
lafrech merged 7 commits intodevfrom
extensible-field2property

Conversation

@Bangertm
Copy link
Copy Markdown
Collaborator

@Bangertm Bangertm commented Jul 22, 2019

I took a shot at making field2property extensible for users to add custom attribute functions that can add attributes to the property dictionary.

  • This pull request extracts the logic for converting fields to properties into a separate class: FieldConverter.
  • Extracts the functionality for handling Nested, List, and Dict fields into separate methods
  • Adds a mechanism for users to add custom functions for handling specific fields or really anything. This test illustrates how a user would be able to extend field2property for a specific field.

I think that this is probably fine for most applications that I have seen in the issues. There are a couple of things I'm not super happy with:

  • Many of the attribute functions we have today depend on being a method so that they can have access to self. For example field2write_only relies on the OpenAPI version number. The logic for handling Nested, List, and Dictionary fields rely on calling other methods resolve_nested_field and field2property. The current mechansim wouldn't allow for that flexibility.
  • Right now I have MarshmallowPlugin initialized with parameters that only needs to pass on to other classes. Might make more sense to allow MarshmallowPlugin to be initialized with an instance of OpenAPIConverter and then update it when init_spec is called. Or maybe there is something else we can do.

Note: I have this pull request targeted to my latest branch to make the changes easier to understand.

@lafrech
Copy link
Copy Markdown
Member

lafrech commented Jul 26, 2019 via email

@Bangertm Bangertm force-pushed the extensible-field2property branch from c7b8924 to c2de7d9 Compare July 31, 2019 14:51
@Bangertm Bangertm force-pushed the rework-name-resolver branch from a594ef7 to d880079 Compare July 31, 2019 14:52
@Bangertm
Copy link
Copy Markdown
Collaborator Author

Updated the method for adding resolver functions and am now providing a way to allow those functions to have access to the FieldConverter instance so that they call other methods like resolve_nested_schema or field2property.

@lafrech
Copy link
Copy Markdown
Member

lafrech commented Aug 21, 2019

Although #476 was merged, GH still presents the whole diff. Do you think you could ensure it only shows the diff from this PR only? You could close and recreate a new PR, but maybe it works by just push-forcing, I don't know.

@lafrech lafrech mentioned this pull request Aug 21, 2019
@Bangertm Bangertm changed the base branch from rework-name-resolver to dev August 21, 2019 15:08
@Bangertm Bangertm force-pushed the extensible-field2property branch from c2de7d9 to e80cebe Compare August 21, 2019 15:12
@Bangertm
Copy link
Copy Markdown
Collaborator Author

Just rebased and forced pushed. I would want to squash the last two commits before merging. Leaving here for now to show multiple ways this could work.

@lafrech
Copy link
Copy Markdown
Member

lafrech commented Aug 27, 2019

I just pushed an additional commit in which I change FieldConverter into FieldConverterMixin.

This makes it closes to former design while still separating responsibilities.

Just a suggestion, we can revert it. Feedback welcome.

@Bangertm if that suits you, I think we can merge.

(There's a typo to fix in field_converter.py s/propetry/property. Can be done in another commit or in a rebase.) Rebased. Fixed.

@lafrech lafrech force-pushed the extensible-field2property branch from 4deeea7 to 3001cda Compare August 29, 2019 13:30
@Bangertm
Copy link
Copy Markdown
Collaborator Author

I like the mixin approach - simpler for the user and less logic with passing around callbacks. Just added some documentation. Can you take a look and make sure it's clear.

Comment thread docs/using_plugins.rst Outdated
@lafrech
Copy link
Copy Markdown
Member

lafrech commented Aug 30, 2019

Yes, I like mixin too. The separation still leaks since OpenAPIConverter must call init_attribute_functions at init, but it's better than nothing.

Looks good to me. Thanks !

You may squash some commits before merging if you wish. I don't mind either way.

Perhaps also add a note to the CHANGELOG if you can. Otherwise, we'll do it later.

@Bangertm Bangertm force-pushed the extensible-field2property branch from d70e004 to 137813b Compare August 30, 2019 18:30
@Bangertm
Copy link
Copy Markdown
Collaborator Author

Ideally I would have extracted the field logic as a mixin originally. Let's just leave all the commits as is...

@lafrech
Copy link
Copy Markdown
Member

lafrech commented Aug 30, 2019

Should we mention half of OpenAPIConverter being extracted as FieldConverterMixin as a breaking change? Normally, OpenAPIConverter is not public API so I'd say we don't care. I can add a note about that in the changelog anyway, even after this is merged.

Also, could it be worth making OpenAPIConverter a mixin as well to make subclassing easier? It can be done after this is merged.

@lafrech lafrech merged commit 50a2cbc into dev Aug 31, 2019
@lafrech lafrech deleted the extensible-field2property branch August 31, 2019 08:57
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.

2 participants