Skip to content

Make serializer fields import explicit #4628

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Nov 1, 2016
Merged

Make serializer fields import explicit #4628

merged 2 commits into from
Nov 1, 2016

Conversation

rpkilby
Copy link
Contributor

@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.

@lovelydinosaur lovelydinosaur added this to the 3.5.2 Release milestone Oct 26, 2016
@lovelydinosaur
Copy link
Contributor

Seems reasonable, yup.

@adamn adamn mentioned this pull request Oct 26, 2016
Closed
@lovelydinosaur
Copy link
Contributor

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
Contributor Author

rpkilby commented Oct 26, 2016

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

@rpkilby
Copy link
Contributor Author

rpkilby commented Oct 26, 2016 •
edited
Loading

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

@lovelydinosaur
Copy link
Contributor

Great. Anyone else got thoughts/review on this?

lovelydinosaur
lovelydinosaur approved these changes Nov 1, 2016
View reviewed changes
tests/test_serializer.py
)

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

Choose a reason for hiding this comment

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

These are great!

@lovelydinosaur lovelydinosaur 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
@dodobas
Fix wrong import of DRF constant
9d3e288 
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
Reviewers

@lovelydinosaur lovelydinosaur lovelydinosaur approved these changes

Assignees
No one assigned
Labels
Cleanup
Projects
None yet
Milestone
3.5.2 Release
Development

Successfully merging this pull request may close these issues.
2 participants
@rpkilby @lovelydinosaur