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

Remove assertions for redundant SerializerMethodField source args. #2422

Closed
wants to merge 1 commit into from
Closed

Remove assertions for redundant SerializerMethodField source args. #2422

wants to merge 1 commit into from

Conversation

gtaylor
Copy link

@gtaylor gtaylor commented Jan 15, 2015

As discussed in #2420, removed SerializerMethodField's assertion raised when redundant source params are passed.

It looks like this was also in the base Field class. For the sake of consistency, I removed this, too. While I think an error could make sense for something like a CharField on a ModelSerializer, it would be a bit of an inconsistent bit of policing. It's probably best left to the people using DRF to police their own codebases. FWIW, I'd definitely pick on redundant CharFields in my code reviews, but SerializerMethodFields are a lot more open-ended by definition.

If you wanted to take the split approach (policing for non-SerializerMethodField), I could add an optional kwarg to bind and have SerializerMethodField's init pass along a value to disable the policing. Though, that could gunk some people up since we'd be changing the signature of a pretty base level class.

The 3.0 release announcement were updated. I thought about adding an entry to the changelog under 3.0.4, but since I don't see a header for 3.0.4 yet I am assuming this is done as part of the release process.

All tests passed on tox, aside from the django-master stuff that was already failing beforehand.

"Remove the `source` keyword argument." %
(field_name, self.__class__.__name__, parent.__class__.__name__)
)

Copy link
Member

Choose a reason for hiding this comment

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

We definatly do want to keep this check. This style is simply unneccessary and confusing.

This:

foo = fields.IntegerField(source='foo')

Is absolutely poor style, and we shouldn't allow it.

@tomchristie
Copy link
Member

I'm willing to consider this for SerializerMethodField, but certainly not for source.

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