Standardizing the use of settings directly #5632
Conversation
saadmk11
changed the title
|
I'd say different PRs |
|
@stsewd can you please see this? #5588 (comment) |
|
If changes are not big, it's fine for me |
|
Thanks. I don't think there will be any big changes :) |
saadmk11
changed the title
|
@stsewd Updated the PR. Can you tell me what should I do about the constant files? |
|
https://github.com/rtfd/readthedocs.org/blob/master/readthedocs/doc_builder/constants.py Docker constants can be moved to the settings file, the mkdocs template should stay (actually I'm removing the mkdocs setting in #5625) These can be moved (also, we can just drop the old version setting support) These can be moved to the settings file too |
| @@ -159,7 +152,6 @@ class BuildConfigBase: | |||
| 'submodules', | |||
| ] | |||
|
|
|||
| default_build_image = DOCKER_DEFAULT_VERSION | |||
There was a problem hiding this comment.
Different configuration files can have a different default image
readthedocs/core/middleware.py
Outdated
| public_domain not in host and production_domain not in host and | ||
| 'localhost' not in host and 'testserver' not in host | ||
| public_domain not in host and settings.PRODUCTION_DOMAIN not in host | ||
| and 'localhost' not in host and 'testserver' not in host |
There was a problem hiding this comment.
we don't start a line with an operator
There was a problem hiding this comment.
Also, maybe this can be written like
if all(test_host not in host for test_host in ('localhost', 'testserver', public_domain, settings.PRODUCTION_DOMAIN)):(not sure if that is more readable)
readthedocs/core/resolver.py
Outdated
| use_https and public_domain and public_domain in domain, | ||
| settings.PUBLIC_DOMAIN_USES_HTTPS | ||
| and settings.PUBLIC_DOMAIN | ||
| and settings.PUBLIC_DOMAIN in domain, |
| cls_name = settings.MESSAGE_STORAGE | ||
| cls = import_string(cls_name) | ||
|
|
||
| cls = import_string(settings.MESSAGE_STORAGE) |
readthedocs/search/documents.py
Outdated
| project_index = Index(project_conf['name']) | ||
| project_index.settings(**project_conf['settings']) | ||
| project_index = Index(settings.ES_INDEXES['project']['name']) | ||
| project_index.settings(**settings.ES_INDEXES['project']['settings']) |
There was a problem hiding this comment.
no need to change this and others
Yes, that's why I wanted to have a different PR, it's easier to review if there aren't many changes, feel free to add those changes in another PR if there are many changes (+10 files maybe?) |
Thats a good idea. I think we should add a new PR for the constants. |
|
@stsewd Updated the PR. did not touch the |
Codecov Report
@@ Coverage Diff @@
## master #5632 +/- ##
==========================================
- Coverage 78.81% 78.81% -0.01%
==========================================
Files 172 172
Lines 10638 10614 -24
Branches 1338 1338
==========================================
- Hits 8384 8365 -19
+ Misses 1907 1902 -5
Partials 347 347
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #5632 +/- ##
==========================================
- Coverage 78.81% 78.81% -0.01%
==========================================
Files 172 172
Lines 10638 10614 -24
Branches 1338 1338
==========================================
- Hits 8384 8365 -19
+ Misses 1907 1902 -5
Partials 347 347
Continue to review full report at Codecov.
|
|
This is starting to get conflicts. Can this be merged or further changes required? |
There are many places that we are using different settings with this kind of pattern. Can we address them all in one PR ? or different PR needed for each.
ref: #5588 (comment)