Use psycopg2-binary in order to remove Django warnings and fix a typo in docs #25

mehdipourfar · 2019-03-23T20:31:56Z

No description provided.

danni · 2019-03-24T21:34:10Z

README.md

+    pip install -r test_requirements.in
+    export PYTHONPATH="$(pwd)"
+    export DJANGO_SETTINGS_MODULE='tests.settings'
+    django-admin test


This is not the accurate. Tests should be run with tox.

I don't have any experience with tox. Please, correct it yourself.

You can copy this directly out of the Travis config: https://github.com/danni/django-postgres-composite-types/blob/master/.travis.yml#L22

https://tox.readthedocs.io/en/latest/

pip install tox tox

Or if you want a specific environment

tox -e py37-dj2.0

While you're at it. Since you're now adding code for Django 2.1, add that to Travis and Tox.ini

danni · 2019-03-24T22:47:49Z

postgres_composite_types/forms.py

@@ -218,6 +218,12 @@ def value_from_datadict(self, data, files, name):
            for subname, widget in self.widgets.items()
        }

+    def value_omitted_from_data(self, data, files, name):
+        for key in data:
+            if key.startswith('{}-'.format(name)):


Calculate the comparison name outside the loop and store in a variable.

It has been rewritten in a new commit.

danni · 2019-03-24T22:48:10Z

postgres_composite_types/forms.py

+        for key in data:
+            if key.startswith('{}-'.format(name)):
+                return False
+        return True


Break after for block.

danni · 2019-03-24T22:50:52Z

postgres_composite_types/forms.py

@@ -146,7 +146,7 @@ def clean(self, value):
                except forms.ValidationError as error:
                    errors.append(prefix_validation_error(
                        error, code='field_invalid',
-                        prefix='%(label)s: ', params={'label': field.label}))
+                        prefix='%(label)s:', params={'label': field.label}))


Hmm, I can't explain this space. I think removing it is correct.

Now I found out that this is due to a change in Django 2.1:
Refs #29131
I have created another commit which will fix it.
make error prefix compatible with different django versions

danni · 2019-03-25T22:09:51Z

postgres_composite_types/forms.py

@@ -144,9 +145,11 @@ def clean(self, value):
                try:
                    cleaned_data[name] = field.clean(value.get(name))
                except forms.ValidationError as error:
+                    prefix = '%(label)s:' if django.__version__ >= '2.1.0' \


Avoid unneeded ternary operator.

Also string version compares will fail where semver and string sorting follow different rules. Use distutils.LooseVersion or compare against the django.VERSION tuple.

e.g. like this Python check:

PY36 = sys.version_info >= (3, 6)

Okay, I have pushed a new commit.
And thanks a lot. I'm learning from you:)

Don't forget to remove the ternary operator. Much clearer just to assign and use a variable.

Why? I didn't understand.

Ternary operators decrease readability, and serve no purpose outside of pure functional code (comprehensions, lambdas, etc.). In fact, for most of its life Python didn't even have a ternary operator.

condition = snake > 12 result = badger if condition else mushroom

vs

if snake > 12 result = badger else: result = condition

The second one is much clearer what result is. Especially if we have to write multiple different conditions to set result.

I don't agree about readability when the condition and result values are very simple. Maybe you have guard against it, simply because you don't use it.

See https://pressupinc.com/blog/2015/03/ternary-operators-considered-harmful/

Consider this case for readability:

result = badger if snake else mushroom if owl else simoncowell

If snake is True and owl is False what's the value of result ?

result is badger because snake is True but I'm against using nested ternary condition. Anyway, I've fix it in a new commit:)

danni · 2019-03-25T22:10:31Z

postgres_composite_types/forms.py

-                return False
-        return True
+        prefix = '{}-'.format(name)
+        return not any(key.startswith(prefix) for key in data)


danni · 2019-03-26T00:37:54Z

Pylint and isort have also generated a few warnings against your code. It looks like at least one of those warnings if something I just fixed in master. So merge master and fix what's left, and it'll be good to go.

…ypes

danni · 2019-03-26T22:09:24Z

tox.ini

@@ -1,7 +1,7 @@
 [tox]
 skipsdist = True
 envlist =
-  py{35,36}-dj{1.11,2.0}
+  py{35,36}-dj{1.11,2.0, 2.1}


Does this space here work correctly?

Yes it works. But I have a problem with tests related to django 1.11. I get the following error:

ModuleNotFoundError: No module named 'tests

And also, when I want to run the tests, I will add Postgres password to my database settings. I don't know how your tests pass without setting it.

Let's keep it consistent and remove it I think. Tox is a dark art sometimes.

I run them against a Docker postgres container. Not sure why 1.11 is failing for you there. It is passing in CI.

danni · 2019-03-27T22:31:50Z

Thanks for this!

mehdipourfar added 5 commits March 24, 2019 00:58

use psycopg2-binary to conform django

d25fcad

fix a typo in docs

17b1735

little fix to pass tests

dbd33ed

add test running docs

ad7dde3

implement value_omitted_from_data function inside widget

b0a90ec

danni requested changes Mar 24, 2019

View reviewed changes

mehdipourfar added 2 commits March 25, 2019 18:07

improve value_omitted_from_data function

8b02d93

make error prefix compatible with different django versions

b4cf8f1

danni reviewed Mar 25, 2019

View reviewed changes

mehdipourfar added 2 commits March 26, 2019 04:33

improve django version checking

0a16fea

add django2.1 to tox and travis and fix test docs

aa28bfa

mehdipourfar added 2 commits March 27, 2019 00:23

Merge branch 'master' of github.com:danni/django-postgres-composite-t…

3a41f04

…ypes

fix pylint errors

ecf62d8

danni reviewed Mar 26, 2019

View reviewed changes

improve code style

4f94272

danni approved these changes Mar 27, 2019

View reviewed changes

danni merged commit 121ecd0 into danni:master Mar 27, 2019

danni mentioned this pull request Mar 27, 2019

django admin form #26

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use psycopg2-binary in order to remove Django warnings and fix a typo in docs #25

Use psycopg2-binary in order to remove Django warnings and fix a typo in docs #25

mehdipourfar commented Mar 23, 2019

danni Mar 24, 2019

mehdipourfar Mar 25, 2019

danni Mar 25, 2019

danni Mar 24, 2019

mehdipourfar Mar 25, 2019 •

edited

Loading

danni Mar 24, 2019

danni Mar 24, 2019

mehdipourfar Mar 25, 2019

danni Mar 25, 2019

mehdipourfar Mar 26, 2019

danni Mar 26, 2019

mehdipourfar Mar 26, 2019

danni Mar 26, 2019

mehdipourfar Mar 26, 2019

danni Mar 26, 2019

mehdipourfar Mar 27, 2019

danni Mar 25, 2019

danni commented Mar 26, 2019

danni Mar 26, 2019

mehdipourfar Mar 26, 2019

danni Mar 26, 2019

danni commented Mar 27, 2019

Use psycopg2-binary in order to remove Django warnings and fix a typo in docs #25

Use psycopg2-binary in order to remove Django warnings and fix a typo in docs #25

Conversation

mehdipourfar commented Mar 23, 2019

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

mehdipourfar Mar 25, 2019 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danni commented Mar 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danni commented Mar 27, 2019

mehdipourfar Mar 25, 2019 •

edited

Loading