Make set_value a method within Serializer by tienne-B · Pull Request #8001 · encode/django-rest-framework (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation14 Commits4 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

tienne-B

As an alternative to #7671, let the method be overridden if needed.

As the function is only used for serializers, it has a better place in the Serializer class than standalone in the fields file.

@tienne-B

As an alternative to encode#7671, let the method be overridden if needed. As the function is only used for serializers, it has a better place in the Serializer class.

blockchainGuru1018

@stale

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@tienne-B

I do think this PR is still pertinent as it would make it so I wouldn't have to override an unrelated method as I currently do.

@stale

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@tienne-B

@stale

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@tomchristie

Okay...

I'm open to considering this. We're pretty much not adding public API to REST framework anymore, but let's take a look.

I'd suggest start off by changing it to not be a staticmethod. Agreed that it functionally is, but we don't use that much through REST framework, so let's stick with consistency.

That edit should also bump the test cases to run against this pull request.

@tienne-B

@auvipy

That edit should also bump the test cases to run against this pull request.

does that mean some additional tests should be added or tests should be adjusted as well?

@tienne-B

These tests follow the examples given in the method.

@tienne-B

Conflicts:

tests/test_serializer.py

@tienne-B

@auvipy There were no tests specifically for set_value() so I've added a couple cases from the method doc.

auvipy

@@ -346,6 +346,26 @@ class Serializer(BaseSerializer, metaclass=SerializerMetaclass):
'invalid': _('Invalid data. Expected a dictionary, but got {datatype}.')
}
def set_value(self, dictionary, keys, value):

Choose a reason for hiding this comment

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

do we need any documentation update // adjustments for the new changes?

Choose a reason for hiding this comment

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

I wouldn't think so, as the change is really just internal and does not affect how set_value() works. Would you want me to document the changes somewhere?

@stale

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@tienne-B

I'm still interested in this, and waiting for a reply from @auvipy.

auvipy

@auvipy

I'm still interested in this, and waiting for a reply from @auvipy.

can you edit the PR headline as per the suggested change by Tom?

@tienne-B tienne-B changed the titleMake set_value a static method for Serializers Make set_value a method within Serializer

May 24, 2023

@tienne-B

can you edit the PR headline as per the suggested change by Tom?

Done :)

saadullahaleem pushed a commit to saadullahaleem/django-rest-framework that referenced this pull request

May 27, 2023

@tienne-B @saadullahaleem

As an alternative to encode#7671, let the method be overridden if needed. As the function is only used for serializers, it has a better place in the Serializer class.

These tests follow the examples given in the method.

saadullahaleem pushed a commit to saadullahaleem/django-rest-framework that referenced this pull request

May 27, 2023

@tienne-B @saadullahaleem

As an alternative to encode#7671, let the method be overridden if needed. As the function is only used for serializers, it has a better place in the Serializer class.

These tests follow the examples given in the method.

auvipy added a commit that referenced this pull request

May 29, 2023

Django URLs of the documentation of reverse and reverse_lazy were wrong.

Co-authored-by: Jorn van Wier jorn.van.wier@thunderbyte.ai Co-authored-by: Asif Saif Uddin auvipy@gmail.com

As an alternative to #7671, let the method be overridden if needed. As the function is only used for serializers, it has a better place in the Serializer class.

These tests follow the examples given in the method.

As an alternative to #7671, let the method be overridden if needed. As the function is only used for serializers, it has a better place in the Serializer class.

These tests follow the examples given in the method.

Co-authored-by: Sergei Shishov sshishov@users.noreply.github.com

Co-authored-by: Sergei Shishov sshishov@users.noreply.github.com

Co-authored-by: Mathieu Dupuy deronnax@gmail.com Co-authored-by: Mehraz Hossain Rumman 59512321+MehrazRumman@users.noreply.github.com Co-authored-by: Dominik Bruhn dominik@dbruhn.de Co-authored-by: jornvanwier mail@jornvanwier.com Co-authored-by: Jorn van Wier jorn.van.wier@thunderbyte.ai Co-authored-by: Asif Saif Uddin auvipy@gmail.com Co-authored-by: Étienne Beaulé beauleetienne0@gmail.com Co-authored-by: Sergei Shishov sshishov@users.noreply.github.com