Skip to content

Match original argument names#5889

Merged
carltongibson merged 1 commit intoencode:masterfrom
decibyte:docs-match-original-argument-names
Apr 20, 2018
Merged

Match original argument names#5889
carltongibson merged 1 commit intoencode:masterfrom
decibyte:docs-match-original-argument-names

Conversation

@decibyte
Copy link
Copy Markdown
Contributor

Description

Update custom field documentation. Change argument names in overridden field methods to match those of the base classes.

I got the following warning from pylint:

Parameters differ from overridden 'to_representation' method (arguments-differ)

Then I returned to the docs just to realise that I did exactly what it said. A look at the source revealed that the warning was actually correct: The name of the second argument to to_representation is value, not obj.

To help others avoid warnings like these, I propose these changes to the docs. But if the argument names have deliberately been altered, feel free to reject this :)

Change argument names in overridden field methods to match those of the base classes.
@xordoquy
Copy link
Copy Markdown
Contributor

Side note, the signature differs in the various classes within the DRF code itself:
to_representation(self, instance)

Copy link
Copy Markdown
Collaborator

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

OK. Yep. Thanks.

@carltongibson carltongibson merged commit 9dbb49e into encode:master Apr 20, 2018
@carltongibson carltongibson added this to the 3.8.3 Release milestone Apr 20, 2018
@rpkilby rpkilby modified the milestones: 3.8.3 Release, 3.9 Release Aug 29, 2018
pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request Nov 17, 2020
Change argument names in overridden field methods to match those of the base classes.
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.

4 participants