Fixed #27546 -- Replaced hardcoded class names in misc. repr() methods. #7629

Keda87 · 2016-11-29T06:01:05Z

https://code.djangoproject.com/ticket/27546

atombrella · 2016-11-29T10:02:54Z

Please reference the ticket number in the commit message. "Fixed #27546 -- ...".

I noticed BaseConverter does not have a test for repr (tests/utils_tests/test_baseconv.py). And for TranslateFile there aren't any tests. For serialization/deserialization, the tests are a bit odd for me.

def test_repr(self):
     base7 = BaseConverter('cjdhel3', sign='g')
     self.assertEqual(repr(base7), '<BaseConverter: base7 (cjdhel3)>')

Keda87 · 2016-11-29T12:01:49Z

@atombrella where I can get convention documentation to write unit testing? I didn't find any in this link https://docs.djangoproject.com/en/dev/internals/contributing/writing-code/unit-tests/
I want to add unit test TranslatableFile class but I avoid to create duplicate file.
also where is test file for serialization/deserialization?

Sorry, it is my first attempt to contribute in django.

timgraham

Try grepping in the tests directory to find places for the tests. e.g. searching for "makemessages" points to tests/i18n. If you can't find a good place for a particular class, let me know.

timgraham · 2016-11-29T12:22:35Z

django/core/management/commands/makemessages.py

@@ -46,7 +46,8 @@ def __init__(self, dirpath, file_name, locale_dir):
        self.locale_dir = locale_dir

    def __repr__(self):
-        return "<TranslatableFile: %s>" % os.sep.join([self.dirpath, self.file])
+        return "<%s: %s>" % (self.__class__.__name__,


We allow lines up to 119 characters if it helps readability, or we use hanging indent like this:

return "<%s: %s>" % ( self.__class__.__name__, ..., )

Keda87 · 2016-11-29T14:09:12Z

@timgraham I've search using makemessages keyword and found testing file under tests/i18n/test_extraction.py
should I create new class in that file? or I should create testing for TranslatableFile in a new file under tests/i18n?

timgraham · 2016-11-29T14:30:46Z

I think i18n/test_management.py would be fine.

timgraham

Did you confirm that all the changes are tested?

timgraham · 2016-11-29T18:24:21Z

django/core/serializers/base.py

-        return "<DeserializedObject: %s(pk=%s)>" % (
-            self.object._meta.label, self.object.pk)
+        return "<%s: %s(pk=%s)>" % (self.__class__.__name__,
+                                    self.object._meta.label, self.object.pk)


Same comment about indentation applies throughout the PR.

I haven't seen tests for this change. I think adding one (whether it's accurate):

def test_deserialized_repr(self): AuthorProfile.objects.create(author=self.joe, date_of_birth=datetime(1970, 1, 1)) serial_str = serializers.serialize(self.serializer_name, AuthorProfile.objects.all()) self.assertFalse(self._get_field_values(serial_str, 'author')) for obj in serializers.deserialize(self.serializer_name, serial_str): self.assertEqual(repr(obj), '<DeserializedObject: AuthorProfile (pk=%s>' % obj.object.pk)

in tests/serializers/tests.py. I don't spot anything else than should be tested.

Keda87 · 2016-11-30T00:37:20Z

Yes I do, I've test by running python tests/runtests.py -v 3 --keepdb

timgraham · 2016-11-30T00:40:01Z

Sorry, I meant are there tests for all the affected _repr__() methods? You can check the coverage report.

Keda87 · 2016-11-30T00:49:40Z

Ohh ya, some of them I haven't create test. I have check the coverage report, but why the report shows the old version code, rather than my changes?

timgraham · 2016-11-30T00:53:29Z

The coverage report runs against master. There isn't any coverage reports for pull requests but it should be easy enough to write the tests without it.

Keda87 · 2016-11-30T02:55:07Z

I have added more test

timgraham

Please split this into two commits: one that adds the tests and one that makes the changes to fix the ticket.

timgraham · 2016-11-30T13:05:21Z

tests/serializers/tests.py

@@ -348,6 +348,18 @@ def test_serialize_proxy_model(self):
        self.assertEqual(base_data, proxy_data.replace('proxy', ''))
        self.assertEqual(base_data, proxy_proxy_data.replace('proxy', ''))

+    def test_deserialized_object_class_repr(self):
+        author = Author.objects.create(name='John Doe')


This seems too complicated, do you really need 3 models? Can you use an in-memory model with its pk set to make the test a bit faster? You could also avoid the need for string interpolation in the expected value if you initialize the model with a pk.

Ohh ya, I don't need 3 models. Previously I thought the m2m_data param is required.
I haven't heard in memory model, is it okay if I only use author model??
I used string interpolation because I don't know the id from newly created model.

timgraham · 2016-11-30T13:05:55Z

tests/i18n/test_management.py

+from django.core.management.commands.makemessages import TranslatableFile
+
+
+class ManagementTest(TestCase):


Test -> Tests

timgraham · 2016-11-30T13:06:15Z

tests/i18n/test_management.py

@@ -0,0 +1,14 @@
+import os
+from unittest import TestCase


Use django.test.SimpleTestCase

timgraham · 2016-11-30T13:07:20Z

django/core/serializers/base.py

-            self.object._meta.label, self.object.pk)
+        return "<%s: %s(pk=%s)>" % (
+            self.__class__.__name__,
+            self.object._meta.label, self.object.pk


put self.object.pk on the next line include a trailing comma

timgraham · 2016-11-30T13:07:28Z

django/core/management/commands/makemessages.py

-        return "<TranslatableFile: %s>" % os.sep.join([self.dirpath, self.file])
+        return "<%s: %s>" % (
+            self.__class__.__name__,
+            os.sep.join([self.dirpath, self.file])


add trailing comma

timgraham · 2016-11-30T13:08:36Z

django/db/migrations/state.py

@@ -588,7 +588,10 @@ def get_index_by_name(self, name):
        raise ValueError("No index named %s on model %s" % (name, self.name))

    def __repr__(self):
-        return "<ModelState: '%s.%s'>" % (self.app_label, self.name)
+        return "<%s: '%s.%s'>" % (


single line looks okay here

do I need add trailing comma?

Not for a single line, the idea is to add one for things like multi-line dicts/tuples so that if more lines are added later, we don't have to modify the last line and add a comma.

timgraham · 2016-11-30T13:09:05Z

tests/raw_query/tests.py

@@ -90,6 +92,12 @@ def assertAnnotations(self, results, expected_annotations):
                self.assertTrue(hasattr(result, annotation))
                self.assertEqual(getattr(result, annotation), value)

+    def test_raw_query_class_repr(self):
+        query = 'SELECT * FROM raw_query_author'
+        raw_queryset_instance = RawQuerySet(raw_query=query)


raw_queryset_instance -> queryset

timgraham · 2016-11-30T17:44:38Z

Looks better. Can you please split the commits as I suggested?

atombrella · 2016-11-29T20:16:52Z

django/core/serializers/base.py

-        return "<DeserializedObject: %s(pk=%s)>" % (
-            self.object._meta.label, self.object.pk)
+        return "<%s: %s(pk=%s)>" % (self.__class__.__name__,
+                                    self.object._meta.label, self.object.pk)


I haven't seen tests for this change. I think adding one (whether it's accurate):

def test_deserialized_repr(self): AuthorProfile.objects.create(author=self.joe, date_of_birth=datetime(1970, 1, 1)) serial_str = serializers.serialize(self.serializer_name, AuthorProfile.objects.all()) self.assertFalse(self._get_field_values(serial_str, 'author')) for obj in serializers.deserialize(self.serializer_name, serial_str): self.assertEqual(repr(obj), '<DeserializedObject: AuthorProfile (pk=%s>' % obj.object.pk)

in tests/serializers/tests.py. I don't spot anything else than should be tested.

Keda87 · 2016-11-30T23:11:03Z

@timgraham sorry out of topic, how to split my commit? I already pushed several commit.

timgraham · 2016-11-30T23:21:23Z

You can:

squash the existing commits
git diff HEAD^1 django/ > ~/a.diff to export the changes in django/ to a diff.
git checkout HEAD~1 django/ && git commit -a --amend to remove the changes in Django from the first commit.
patch -p1 -i ~/a.diff to reapply changes from the diff
git commit -a to create a second commit with the changes in django/
Force push to your branch with the new commits.

Keda87 · 2016-12-01T03:35:59Z

I cannot split it into two commits. I'm only make it in one commit.
I'm rebasing all my commits using g rebase -i HEAD~9 then I squash commit 2-9 and all commit merged in my first commit.

[update]
I've splitted into 2 commits 😁

Keda87 · 2016-12-01T13:15:47Z

@timgraham I saw that some of my test didn't pass CI build because hardcoded ID on the expected result. should I use string interpolation again?

timgraham · 2016-12-01T13:16:45Z

I updated the test to use an in-memory model as I suggested earlier. I think this should be good now, I'll just run the tests once more to confirm all is well.

Keda87 · 2016-12-01T13:19:25Z

ahh TIL, I just know this trick. thanks!

Keda87 force-pushed the replace_hardcoded_repr branch from 330e230 to 9bceccd Compare November 29, 2016 11:43

timgraham reviewed Nov 29, 2016

View reviewed changes

timgraham reviewed Nov 30, 2016

View reviewed changes

timgraham changed the title ~~Fixed #27546 -- Replace hardcoded class names in __repr__-methods~~ Fixed #27546 -- Replaced hardcoded class names in misc. __repr__() methods. Nov 30, 2016

atombrella approved these changes Nov 30, 2016

View reviewed changes

Keda87 force-pushed the replace_hardcoded_repr branch from 6a707d3 to f27feae Compare December 1, 2016 04:23

Refs #27546 -- Tested some __repr__() methods.

794b7d8

timgraham force-pushed the replace_hardcoded_repr branch from f27feae to 32d198a Compare December 1, 2016 13:10

Fixed #27546 -- Removed hardcoded class names in __repr__() methods.

48826aa

timgraham force-pushed the replace_hardcoded_repr branch from 32d198a to 48826aa Compare December 1, 2016 13:11

timgraham merged commit 48826aa into django:master Dec 1, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed #27546 -- Replaced hardcoded class names in misc. repr() methods. #7629

Fixed #27546 -- Replaced hardcoded class names in misc. repr() methods. #7629

Keda87 commented Nov 29, 2016

atombrella commented Nov 29, 2016

Keda87 commented Nov 29, 2016

timgraham left a comment

timgraham Nov 29, 2016

Keda87 commented Nov 29, 2016

timgraham commented Nov 29, 2016

timgraham left a comment

timgraham Nov 29, 2016

atombrella Nov 29, 2016

Keda87 commented Nov 30, 2016

timgraham commented Nov 30, 2016

Keda87 commented Nov 30, 2016

timgraham commented Nov 30, 2016

Keda87 commented Nov 30, 2016

timgraham left a comment

timgraham Nov 30, 2016

Keda87 Nov 30, 2016

timgraham Nov 30, 2016

timgraham Nov 30, 2016

timgraham Nov 30, 2016

timgraham Nov 30, 2016

timgraham Nov 30, 2016

Keda87 Nov 30, 2016

timgraham Nov 30, 2016

timgraham Nov 30, 2016

timgraham commented Nov 30, 2016

atombrella Nov 29, 2016

Keda87 commented Nov 30, 2016

timgraham commented Nov 30, 2016

Keda87 commented Dec 1, 2016 •

edited

Loading

Keda87 commented Dec 1, 2016

timgraham commented Dec 1, 2016

Keda87 commented Dec 1, 2016

		from django.core.management.commands.makemessages import TranslatableFile


		class ManagementTest(TestCase):

Fixed #27546 -- Replaced hardcoded class names in misc. __repr__() methods. #7629

Fixed #27546 -- Replaced hardcoded class names in misc. __repr__() methods. #7629

Conversation

Keda87 commented Nov 29, 2016

atombrella commented Nov 29, 2016

Keda87 commented Nov 29, 2016

timgraham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Keda87 commented Nov 29, 2016

timgraham commented Nov 29, 2016

timgraham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Keda87 commented Nov 30, 2016

timgraham commented Nov 30, 2016

Keda87 commented Nov 30, 2016

timgraham commented Nov 30, 2016

Keda87 commented Nov 30, 2016

timgraham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timgraham commented Nov 30, 2016

Choose a reason for hiding this comment

Keda87 commented Nov 30, 2016

timgraham commented Nov 30, 2016

Keda87 commented Dec 1, 2016 • edited Loading

Keda87 commented Dec 1, 2016

timgraham commented Dec 1, 2016

Keda87 commented Dec 1, 2016

Fixed #27546 -- Replaced hardcoded class names in misc. repr() methods. #7629

Fixed #27546 -- Replaced hardcoded class names in misc. repr() methods. #7629

Keda87 commented Dec 1, 2016 •

edited

Loading