Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from Apr 25, 2019

Conversation

@saadmk11
Copy link
Member

@saadmk11 saadmk11 commented Apr 12, 2019

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

Copy link
Member

@stsewd stsewd left a comment

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

readthedocs/builds/syncers.py Show resolved Hide resolved
@@ -42,11 +44,18 @@ class CommunityBaseSettings(Settings):
PUBLIC_DOMAIN = None
PUBLIC_DOMAIN_USES_HTTPS = False
USE_SUBDOMAIN = False
USE_SUBDOMAINS = None
Copy link
Member

@stsewd stsewd Apr 12, 2019

This is already set just above

Copy link
Member Author

@saadmk11 saadmk11 Apr 12, 2019

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

Copy link
Member

@stsewd stsewd Apr 12, 2019

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

Copy link
Member Author

@saadmk11 saadmk11 Apr 13, 2019

@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

Copy link
Member

@humitos humitos Apr 13, 2019

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

Copy link
Member Author

@saadmk11 saadmk11 Apr 13, 2019

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

Copy link
Member

@stsewd stsewd Apr 13, 2019

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.

Copy link
Member Author

@saadmk11 saadmk11 Apr 13, 2019

Cool 💯

Copy link
Member Author

@saadmk11 saadmk11 Apr 13, 2019

@stsewd Updated the PR

readthedocs/settings/base.py Show resolved Hide resolved
readthedocs/settings/base.py Outdated Show resolved Hide resolved
@saadmk11
Copy link
Member Author

@saadmk11 saadmk11 commented Apr 13, 2019

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

)
DOCKER_IMAGE_SETTINGS = getattr(settings, 'DOCKER_IMAGE_SETTINGS', {})
DOCKER_IMAGE = settings.DOCKER_IMAGE
DOCKER_IMAGE_SETTINGS = settings.DOCKER_IMAGE_SETTINGS
Copy link
Member

@humitos humitos Apr 13, 2019

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

Copy link
Member Author

@saadmk11 saadmk11 Apr 13, 2019

@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?

Copy link
Member

@stsewd stsewd Apr 13, 2019

I vote to do that in another PR

@davidfischer
Copy link
Contributor

@davidfischer davidfischer commented Apr 13, 2019

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!

@@ -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,
Copy link
Member

@stsewd stsewd Apr 13, 2019

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

Copy link
Member Author

@saadmk11 saadmk11 Apr 13, 2019

sure!

Copy link
Member Author

@saadmk11 saadmk11 Apr 13, 2019

@stsewd Done.:)

@stsewd
Copy link
Member

@stsewd stsewd commented Apr 13, 2019

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
Copy link
Member Author

@saadmk11 saadmk11 commented Apr 13, 2019

We still to change more code

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

stsewd
stsewd approved these changes Apr 23, 2019
Copy link
Member

@stsewd stsewd left a comment

Thanks. We should merge this before it generates conflicts

@stsewd stsewd requested a review from Apr 24, 2019
Copy link
Member

@humitos humitos left a comment

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

@humitos
Copy link
Member

@humitos humitos commented Apr 25, 2019

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.

@humitos humitos merged commit 4f206ab into readthedocs:master Apr 25, 2019
1 check passed
@saadmk11 saadmk11 deleted the unify-settings branch Apr 25, 2019
@ericholscher
Copy link
Member

@ericholscher ericholscher commented Apr 25, 2019

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
Copy link
Member

@ericholscher ericholscher commented Apr 25, 2019

This is fine for now though 👍

@saadmk11
Copy link
Member Author

@saadmk11 saadmk11 commented Apr 25, 2019

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
Copy link
Member

@humitos humitos commented Apr 25, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants