Kn/settings rewrite #203

KaczuH · 2017-06-21T12:55:46Z

I saw a few problems with how we handle settings in Djoser. And decided to try solve some of them.

settings.get("SEND_ACTIVATION_EMAIL") is cumbersome interface and it is used a lot across the code. We should look for more clear and simpler way. Thats why I decided to use config.SEND_ACTIVATION_EMAIL somewhat resembling how Django does.
merge_settings_dicts was invoked every time an setting value is looked up. This is not optimal. Additionaly djoser settings aren't so complex so why to use this recursive function? I removed it and replaced with simple approach: instantiate default djoser settings and override anything that user customized in django settings.

There is some work to do but i want to show this idea for consideration.

merge_settings_dicts was invoked always when settings value was fetched. Recursive, hard to read algorithm is not necessary on such simple task getting value from dict.

ConorMcGee · 2017-06-21T12:58:55Z

For what it's worth, I'm definitely -1 on this. I think it's better to stick with the existing code which more closely resembles established Django patterns.

coveralls · 2017-06-21T13:02:23Z

Coverage increased (+0.3%) to 96.627% when pulling b2f0774 on KaczuH:KN/settings_rewrite into e10d916 on sunscrapers:master.

KaczuH · 2017-06-21T13:06:51Z

This is whole point of this rewrite. Current settings interface is in no way resembling django practices

pawelswiecki · 2017-06-21T13:09:48Z

Why merge_settings_dicts was there is the first place?

coveralls · 2017-06-22T18:15:46Z

Coverage decreased (-0.6%) to 95.714% when pulling ca7af88 on KaczuH:KN/settings_rewrite into e10d916 on sunscrapers:master.

haxoza · 2017-06-23T08:16:39Z

djoser/serializers.py

-        return user
+        if config.SEND_ACTIVATION_EMAIL:
+            validated_data.update({'is_active': False})
+        return User.objects.create_user(**validated_data)


AFAIK we cannot get rid of with transaction.atomic(): context manager here because perform_create method is called by create method where IntegrityError is handled. If we do not use separate transaction here then (depending on global django settings) the current transaction (e.g. per request) can be not valid anymore (e.g. for postgres).

See 8a28b76

But in general good idea with shortening the code with validated_data.update({'is_active': False}) 👏

haxoza · 2017-06-23T08:31:12Z

djoser/serializers.py

+                user.is_active = False
+                user.save(update_fields=['is_active'])
+        return user
+


It could be almost like you did:

with transaction.atomic(): if config.SEND_ACTIVATION_EMAIL: validated_data.update({'is_active': False}) return User.objects.create_user(**validated_data)

haxoza · 2017-06-23T08:33:43Z

@pawelswiecki good catch! merge_settings_dicts was there in order to allow djoser users easy default settings overriding. After current changes what user should do if she/he wants to change only one serializer?

KaczuH · 2017-06-23T08:37:26Z

@haxoza Overriding settings is not changed from user perspective. Just define

DJOSER = { 'SERIALIZERS': { 'user': <user_custom_serializer> } }

And only this one serializer is overriden default settings are intact.

coveralls · 2017-06-23T08:40:01Z

Coverage decreased (-0.6%) to 95.745% when pulling 2691082 on KaczuH:KN/settings_rewrite into e10d916 on sunscrapers:master.

pszpetkowski · 2017-06-23T09:07:31Z

djoser/settings.py

+        self._wrapped = Settings(default_settings, explicit_overriden_settings)
+
+
+config = LazySettings()


config name seems very un-pythonic to me. Maybe something like djoser_settings would be better?

So, do we change config to djoser_settings or settings?

settings sound good in my opinion

also one more different idea: what about djoser.conf.settings like Django does django.conf.settings?

@haxoza @piotr-szpetkowski djoser.conf.settings would be the best but it will introduce incompatibility for older versions.
I'll add deprecation warnings and try to keep old interface but it should be dropped in the future.

pszpetkowski · 2017-06-23T09:10:25Z

@mcgeeco care to elaborate what exactly is wrong with idea provided in this PR?

ConorMcGee · 2017-06-23T09:12:12Z

@piotr-szpetkowski No I see what the idea is now, it makes sense - I mainly thought it wasn't useful to go from settings to config as people might be used to looking for settings when they're overriding things.

haxoza · 2017-06-23T09:13:14Z

See also #155

coveralls · 2017-06-23T11:38:25Z

Coverage decreased (-0.8%) to 95.519% when pulling 85c73b2 on KaczuH:KN/settings_rewrite into e10d916 on sunscrapers:master.

pszpetkowski

It would be nice to change few things. Also it would probably by a good idea to write some tests for new settings and update documentation on settings.

pszpetkowski · 2017-06-23T12:54:27Z

djoser/settings.py

+        self._wrapped = Settings(default_settings, explicit_overriden_settings)
+
+
+config = LazySettings()


settings sound good in my opinion

pszpetkowski · 2017-06-23T12:55:47Z

djoser/settings.py

+    This function is here only to provide backwards compatibility in case anyone uses old settings interface.
+    It is strongly encouraged to use dot notation.
+    """
+    warnings.warn('The settings.get(key) is superseded by the dot attribute access.')


How about using deprecation warning?

@piotr-szpetkowski I'll add PendingDeprecationWarning

pszpetkowski · 2017-06-23T12:57:05Z

djoser/settings.py

-    except KeyError:
-        raise ImproperlyConfigured('Missing settings: DJOSER[\'{}\']'.format(key))
+        return getattr(config, key)
+    except ArithmeticError:


Are you sure it's ArithmeticError? Doesn't make much sense to me.

…n bug

…N/settings_rewrite

coveralls · 2017-06-26T14:02:59Z

Coverage decreased (-0.8%) to 95.632% when pulling 9f4e073 on KaczuH:KN/settings_rewrite into 923a0f9 on sunscrapers:master.

coveralls · 2017-06-30T10:05:17Z

Coverage decreased (-0.3%) to 96.092% when pulling d7ecd3c on KaczuH:KN/settings_rewrite into 923a0f9 on sunscrapers:master.

coveralls · 2017-06-30T10:18:08Z

Coverage increased (+0.3%) to 96.782% when pulling 0527246 on KaczuH:KN/settings_rewrite into 923a0f9 on sunscrapers:master.

pszpetkowski

Good to merge in my opinion.

Kamil Niski added 4 commits June 21, 2017 14:09

Add Django-like settinggs class instead of raw dict

7c9dd5d

Remove inefficient unnecessary complex merge_settings_dicts

66c6fbb

merge_settings_dicts was invoked always when settings value was fetched. Recursive, hard to read algorithm is not necessary on such simple task getting value from dict.

Cleanup ipdb brakpoints

f8d03a2

Clean up LazySettings class

b2f0774

Provide backwards compatibility for settings access

ca7af88

haxoza reviewed Jun 23, 2017

View reviewed changes

Revert refactor of user creation due to Django incompatibility

2691082

haxoza reviewed Jun 23, 2017

View reviewed changes

pszpetkowski reviewed Jun 23, 2017

View reviewed changes

Add warning to settings.get

85c73b2

pszpetkowski suggested changes Jun 23, 2017

View reviewed changes

Kamil Niski and others added 7 commits June 26, 2017 11:27

Fix mistyped AttributeError

e48fa85

Add PendingDeprecationWarning to old settngs interface

5ae28db

Make djoser.conf.settings

8060a0a

Fix typo config -> conf

56c78cf

Merge branch 'master' into KN/settings_rewrite

3e017f6

Rename import of django settings -> django_settings to avoid recursio…

2028c2d

…n bug

Merge branch 'KN/settings_rewrite' of github.com:KaczuH/djoser into K…

090c858

…N/settings_rewrite

Fix broken settings import

9f4e073

Add tests to new settings

d7ecd3c

Test legacy setting method

0527246

pszpetkowski approved these changes Jun 30, 2017

View reviewed changes

pszpetkowski merged commit 7442eb7 into sunscrapers:master Jun 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kn/settings rewrite #203

Kn/settings rewrite #203

KaczuH commented Jun 21, 2017 •

edited

Loading

ConorMcGee commented Jun 21, 2017

coveralls commented Jun 21, 2017

KaczuH commented Jun 21, 2017

pawelswiecki commented Jun 21, 2017

coveralls commented Jun 22, 2017

haxoza Jun 23, 2017 •

edited

Loading

haxoza Jun 23, 2017

haxoza commented Jun 23, 2017

KaczuH commented Jun 23, 2017

coveralls commented Jun 23, 2017 •

edited

Loading

pszpetkowski Jun 23, 2017

haxoza Jun 23, 2017

pszpetkowski Jun 23, 2017 •

edited

Loading

haxoza Jun 23, 2017

KaczuH Jun 26, 2017

pszpetkowski commented Jun 23, 2017

ConorMcGee commented Jun 23, 2017

haxoza commented Jun 23, 2017

coveralls commented Jun 23, 2017 •

edited

Loading

pszpetkowski left a comment

pszpetkowski Jun 23, 2017 •

edited

Loading

pszpetkowski Jun 23, 2017

KaczuH Jun 26, 2017

pszpetkowski Jun 23, 2017 •

edited

Loading

coveralls commented Jun 26, 2017 •

edited

Loading

coveralls commented Jun 30, 2017 •

edited

Loading

coveralls commented Jun 30, 2017 •

edited

Loading

pszpetkowski left a comment

		self._wrapped = Settings(default_settings, explicit_overriden_settings)


		config = LazySettings()

Kn/settings rewrite #203

Kn/settings rewrite #203

Conversation

KaczuH commented Jun 21, 2017 • edited Loading

ConorMcGee commented Jun 21, 2017

coveralls commented Jun 21, 2017

KaczuH commented Jun 21, 2017

pawelswiecki commented Jun 21, 2017

coveralls commented Jun 22, 2017

haxoza Jun 23, 2017 • edited Loading

Choose a reason for hiding this comment

haxoza Jun 23, 2017

Choose a reason for hiding this comment

haxoza commented Jun 23, 2017

KaczuH commented Jun 23, 2017

coveralls commented Jun 23, 2017 • edited Loading

pszpetkowski Jun 23, 2017

Choose a reason for hiding this comment

haxoza Jun 23, 2017

Choose a reason for hiding this comment

pszpetkowski Jun 23, 2017 • edited Loading

Choose a reason for hiding this comment

haxoza Jun 23, 2017

Choose a reason for hiding this comment

KaczuH Jun 26, 2017

Choose a reason for hiding this comment

pszpetkowski commented Jun 23, 2017

ConorMcGee commented Jun 23, 2017

haxoza commented Jun 23, 2017

coveralls commented Jun 23, 2017 • edited Loading

pszpetkowski left a comment

Choose a reason for hiding this comment

pszpetkowski Jun 23, 2017 • edited Loading

Choose a reason for hiding this comment

pszpetkowski Jun 23, 2017

Choose a reason for hiding this comment

KaczuH Jun 26, 2017

Choose a reason for hiding this comment

pszpetkowski Jun 23, 2017 • edited Loading

Choose a reason for hiding this comment

coveralls commented Jun 26, 2017 • edited Loading

coveralls commented Jun 30, 2017 • edited Loading

coveralls commented Jun 30, 2017 • edited Loading

pszpetkowski left a comment

Choose a reason for hiding this comment

KaczuH commented Jun 21, 2017 •

edited

Loading

haxoza Jun 23, 2017 •

edited

Loading

coveralls commented Jun 23, 2017 •

edited

Loading

pszpetkowski Jun 23, 2017 •

edited

Loading

coveralls commented Jun 23, 2017 •

edited

Loading

pszpetkowski Jun 23, 2017 •

edited

Loading

pszpetkowski Jun 23, 2017 •

edited

Loading

coveralls commented Jun 26, 2017 •

edited

Loading

coveralls commented Jun 30, 2017 •

edited

Loading

coveralls commented Jun 30, 2017 •

edited

Loading