Unify usage of settings and remove the usage of getattr for settings #5588

saadmk11 · 2019-04-12T08:22:48Z

I wasn't quite sure what to with this so I left it as it is:
https://github.com/rtfd/readthedocs.org/blob/7fa9002586b2c5c2fae5c78e5ad736d9d46f7ad4/readthedocs/core/utils/extend.py#L29

closes #2140

stsewd

I just did a quick review, we need to check more closely to some default values.

readthedocs/builds/syncers.py

stsewd · 2019-04-12T13:55:19Z

readthedocs/settings/base.py

@@ -42,11 +44,18 @@ class CommunityBaseSettings(Settings):
    PUBLIC_DOMAIN = None
    PUBLIC_DOMAIN_USES_HTTPS = False
    USE_SUBDOMAIN = False
+    USE_SUBDOMAINS = None


This is already set just above

I was a bit confused about this. one is USE_SUBDOMAIN and the other one is USE_SUBDOMAINS Which is called here:
https://github.com/rtfd/readthedocs.org/blob/3d36585aca80e7f79b0c5bfeb1e4f6f1e38b8f06/readthedocs/core/context_processors.py#L13

oh, I just realized of the final s. We should investigate a liitle more here.

@stsewd I did some investigation on my own. I found that this settings is only being used by context_processors.py https://github.com/rtfd/readthedocs.org/blob/7fa9002586b2c5c2fae5c78e5ad736d9d46f7ad4/readthedocs/core/context_processors.py#L13

I suppose this was a typo and the intended usage was USE_SUBDOMAIN.

@humitos I thought that too. Should I Change this to USE_SUBDOMAIN and Update the PR?

Looks like most of the code and tests we use USE_SUBDOMAIN, also I search the whole project (and the corporate site), it's safe to rename it in the context processor file.

@stsewd Updated the PR

readthedocs/settings/base.py

saadmk11 · 2019-04-13T09:22:36Z

@stsewd Updated the PR with the changes you suggested :)

humitos · 2019-04-13T14:17:28Z

readthedocs/config/config.py

-)
-DOCKER_IMAGE_SETTINGS = getattr(settings, 'DOCKER_IMAGE_SETTINGS', {})
+DOCKER_IMAGE = settings.DOCKER_IMAGE
+DOCKER_IMAGE_SETTINGS = settings.DOCKER_IMAGE_SETTINGS


Why we are re-defining these here instead of using them directly from the settings. object in the code?

@humitos I did not change it as it was already like this. And also they are being called in many parts of the config.py file. should I use them directly from the settings in the places of config.py?

I vote to do that in another PR

davidfischer · 2019-04-13T19:22:57Z

I just wanted to chime in that I'm excited for this change. This will simplify a lot of things and centralize all our settings in one place so they are easier to understand!

stsewd · 2019-04-13T20:08:03Z

readthedocs/core/context_processors.py

@@ -10,7 +10,7 @@ def readthedocs_processor(request):
    exports = {
        'PUBLIC_DOMAIN': settings.PUBLIC_DOMAIN,
        'PRODUCTION_DOMAIN': settings.PRODUCTION_DOMAIN,
-        'USE_SUBDOMAINS': settings.USE_SUBDOMAINS,
+        'USE_SUBDOMAINS': settings.USE_SUBDOMAIN,


We should rename the key too (I just checked, and there is not other places using this)

@stsewd Done.:)

stsewd

We still need to change more code

https://github.com/rtfd/readthedocs.org/blob/91994c25d381d648f8f06783607f0304d149831f/readthedocs/doc_builder/loader.py#L10-L23

https://github.com/rtfd/readthedocs.org/blob/91994c25d381d648f8f06783607f0304d149831f/readthedocs/builds/models.py#L52-L56

stsewd · 2019-04-13T20:59:42Z

I wasn't quite sure what to with this so I left it as it is:
https://github.com/rtfd/readthedocs.org/blob/7fa9002586b2c5c2fae5c78e5ad736d9d46f7ad4/readthedocs/core/utils/extend.py#L29

That should remain as it is.

saadmk11 · 2019-04-13T21:16:55Z

We still to change more code

@stsewd I might have missed these two from my search. Updated the PR! Thanks.

stsewd

Thanks. We should merge this before it generates conflicts

humitos

Thanks @saadmk11 for your work on this. Changes look good to me.

humitos · 2019-04-25T11:09:07Z

This PR touches many different files. I'm merging it now to avoid conflicts and also to start testing it locally considering that there are many settings re-defined. Although, the defaults on base.py looks accurate and the same as the ones used by getattr. I'm not expecting to fail on prod, but we should be careful when deploying it.

ericholscher · 2019-04-25T13:07:12Z

I think we should probably do a followup here to remove places where we're using sync_user = settings.SYNC_USER to use the setting directly. We're doing this a number of places in the code, and I think it's a bad pattern.

ericholscher · 2019-04-25T13:07:42Z

This is fine for now though 👍

saadmk11 · 2019-04-25T13:08:49Z

I think we should probably do a followup here to remove places where we're using sync_user = settings.SYNC_USER to use the setting directly. We're doing this a number of places in the code, and I think it's a bad pattern.

@ericholscher Okay! I will start working on it:)

humitos · 2019-04-25T15:05:04Z

I think we should probably do a followup here to remove places where we're using sync_user = settings.SYNC_USER to use the setting directly. We're doing this a number of places in the code, and I think it's a bad pattern.

@ericholscher @saadmk11 I already mentioned this at #5588 (comment)

I think the new PR should include all the changes for this pattern.

saadmk11 added 2 commits April 12, 2019 14:16

Unify usage of settings to base.py

95c99e9

lint fix

3d36585

stsewd reviewed Apr 12, 2019

View reviewed changes

Updated settings

1321c69

humitos reviewed Apr 13, 2019

View reviewed changes

Typo Fixed

18de1fc

stsewd reviewed Apr 13, 2019

View reviewed changes

Key name changed.

91994c2

davidfischer mentioned this pull request Apr 13, 2019

Move our application settings to a READTHEDOCS prefix on our settings #5586

Open

stsewd reviewed Apr 13, 2019

View reviewed changes

Added more settings to base

4358f8b

stsewd approved these changes Apr 23, 2019

View reviewed changes

stsewd requested a review from a team April 24, 2019 16:07

humitos approved these changes Apr 25, 2019

View reviewed changes

humitos merged commit 4f206ab into readthedocs:master Apr 25, 2019

saadmk11 deleted the unify-settings branch April 25, 2019 11:10

saadmk11 mentioned this pull request Apr 25, 2019

Standardizing the use of settings directly #5632

Merged

yakky mentioned this pull request Oct 18, 2019

[upstream] Creare PR port modifiche constants italia/docs.italia.it#297

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify usage of settings and remove the usage of getattr for settings #5588

Unify usage of settings and remove the usage of getattr for settings #5588

saadmk11 commented Apr 12, 2019

stsewd left a comment

stsewd Apr 12, 2019

saadmk11 Apr 12, 2019 •

edited

Loading

stsewd Apr 12, 2019

saadmk11 Apr 13, 2019

humitos Apr 13, 2019

saadmk11 Apr 13, 2019

stsewd Apr 13, 2019

saadmk11 Apr 13, 2019

saadmk11 Apr 13, 2019

saadmk11 commented Apr 13, 2019

humitos Apr 13, 2019

saadmk11 Apr 13, 2019

stsewd Apr 13, 2019

davidfischer commented Apr 13, 2019

stsewd Apr 13, 2019

saadmk11 Apr 13, 2019

saadmk11 Apr 13, 2019

stsewd left a comment •

edited

Loading

stsewd commented Apr 13, 2019

saadmk11 commented Apr 13, 2019 •

edited

Loading

stsewd left a comment

humitos left a comment

humitos commented Apr 25, 2019

ericholscher commented Apr 25, 2019

ericholscher commented Apr 25, 2019

saadmk11 commented Apr 25, 2019

humitos commented Apr 25, 2019

Unify usage of settings and remove the usage of getattr for settings #5588

Unify usage of settings and remove the usage of getattr for settings #5588

Conversation

saadmk11 commented Apr 12, 2019

stsewd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saadmk11 Apr 12, 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

saadmk11 commented Apr 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfischer commented Apr 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stsewd left a comment • edited Loading

Choose a reason for hiding this comment

stsewd commented Apr 13, 2019

saadmk11 commented Apr 13, 2019 • edited Loading

stsewd left a comment

Choose a reason for hiding this comment

humitos left a comment

Choose a reason for hiding this comment

humitos commented Apr 25, 2019

ericholscher commented Apr 25, 2019

ericholscher commented Apr 25, 2019

saadmk11 commented Apr 25, 2019

humitos commented Apr 25, 2019

saadmk11 Apr 12, 2019 •

edited

Loading

stsewd left a comment •

edited

Loading

saadmk11 commented Apr 13, 2019 •

edited

Loading