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

Make serializer fields import explicit #4628

Merged
merged 2 commits into from Nov 1, 2016

Conversation

rpkilby
Copy link
Member

@rpkilby rpkilby commented Oct 26, 2016

This is a better approach to fixing the from fields import * in serializers (see #4626/#4613).

  • Field class imports are separated from utility imports (SkipField, empty, etc...)
  • The test ensures all fields & related fields are imported into the serializers module. New field classes won't accidentally be skipped.

@tomchristie tomchristie added this to the 3.5.2 Release milestone Oct 26, 2016
@tomchristie
Copy link
Member

Seems reasonable, yup.

@adamn adamn mentioned this pull request Oct 26, 2016
@tomchristie
Copy link
Member

Looking good - linter failed on isort, otherwise happy with it.
Any thoughts on if there's likely gotchas with users importing private functions in fields, using the serializers.* notation that we wouldn't now support? What isn't in the import anymore that used to be, that some users might be working with?

@rpkilby
Copy link
Member Author

rpkilby commented Oct 26, 2016

from foo import * ignores objects where the name has a leading _.

@rpkilby
Copy link
Member Author

rpkilby commented Oct 26, 2016

See the latest commit - added a section for non-field imports that are still public API.


All of the non-field definitions:

# fields.py
empty
is_simple_callable
get_attribute
set_value
to_choices_dict
flatten_choices_dict
iter_options
get_error_detail
CreateOnlyDefault
CurrentUserDefault
SkipField
REGEX_TYPE
NOT_READ_ONLY_WRITE_ONLY
NOT_READ_ONLY_REQUIRED
NOT_REQUIRED_DEFAULT
USE_READONLYFIELD
MISSING_ERROR_MESSAGE

# relations.py
method_overridden
Hyperlink
PKOnlyObject
MANY_RELATION_KWARGS

From the above, serializers currently imports:

# fields.py
empty
get_error_detail
set_value
CreateOnlyDefault
SkipField

# relations.py
Hyperlink
PKOnlyObject

@tomchristie
Copy link
Member

Great. Anyone else got thoughts/review on this?

)

def test_fields(self):
msg = "Expected `fields.%s` to be imported in `serializers`"
Copy link
Member

Choose a reason for hiding this comment

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

These are great!

@tomchristie tomchristie merged commit d92b24a into encode:master Nov 1, 2016
@rpkilby rpkilby deleted the fix-fields-import branch September 25, 2017 18:47
dodobas added a commit to nyaruka/rapidpro that referenced this pull request Mar 5, 2018
We are stuck on DRF 3.5.1 because on 3.5.2 this PR got merged
encode/django-rest-framework#4628
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants