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

Standardizing the use of settings directly #5632

Merged
merged 11 commits into from May 9, 2019

Conversation

@saadmk11
Copy link
Member

@saadmk11 saadmk11 commented Apr 25, 2019

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)

@saadmk11 saadmk11 changed the title calling SYNC_USER settings directly Using SYNC_USER settings directly Apr 25, 2019
@stsewd
Copy link
Member

@stsewd stsewd commented Apr 25, 2019

I'd say different PRs

#5588 (comment)

@saadmk11
Copy link
Member Author

@saadmk11 saadmk11 commented Apr 25, 2019

@stsewd can you please see this? #5588 (comment)

@stsewd
Copy link
Member

@stsewd stsewd commented Apr 25, 2019

If changes are not big, it's fine for me

@saadmk11
Copy link
Member Author

@saadmk11 saadmk11 commented Apr 25, 2019

Thanks. I don't think there will be any big changes :)

@saadmk11 saadmk11 changed the title Using SYNC_USER settings directly Standardizing the use of settings directly Apr 25, 2019
@saadmk11
Copy link
Member Author

@saadmk11 saadmk11 commented Apr 25, 2019

@stsewd
Copy link
Member

@stsewd stsewd commented Apr 25, 2019

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

https://github.com/rtfd/readthedocs.org/blob/cd5476be78dfe98cf538adfbf4801b51ce1a66be/readthedocs/doc_builder/constants.py#L24-L37

(also, we can just drop the old version setting support)

These can be moved to the settings file too

https://github.com/rtfd/readthedocs.org/blob/cd5476be78dfe98cf538adfbf4801b51ce1a66be/readthedocs/builds/constants.py#L44-L48

@@ -159,7 +152,6 @@ class BuildConfigBase:
'submodules',
]

default_build_image = DOCKER_DEFAULT_VERSION
Copy link
Member

@stsewd stsewd Apr 25, 2019

I think this one can stay

Copy link
Member

@stsewd stsewd Apr 25, 2019

Different configuration files can have a different default image

Copy link
Member Author

@saadmk11 saadmk11 Apr 25, 2019

Okay

@saadmk11
Copy link
Member Author

@saadmk11 saadmk11 commented Apr 25, 2019

@stsewd All of those settings were moved to base settings file on #5588
But these constants a being used by many files. should I remove these constants from the constants.py file and just call them from settings file using settings. in the other files where constants were being used?

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

@stsewd stsewd Apr 25, 2019

we don't start a line with an operator

Copy link
Member

@stsewd stsewd Apr 25, 2019

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)

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

@stsewd stsewd Apr 25, 2019

we don't start with an operator

cls_name = settings.MESSAGE_STORAGE
cls = import_string(cls_name)

cls = import_string(settings.MESSAGE_STORAGE)
Copy link
Member

@stsewd stsewd Apr 25, 2019

We are losing readability here

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'])
Copy link
Member

@stsewd stsewd Apr 25, 2019

no need to change this and others

@stsewd
Copy link
Member

@stsewd stsewd commented Apr 25, 2019

But these constants a being used by many files. should I remove these constants from the constants.py file and just call them from settings file using settings. in the other files where constants were being used?

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

@saadmk11
Copy link
Member Author

@saadmk11 saadmk11 commented Apr 25, 2019

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.

@saadmk11
Copy link
Member Author

@saadmk11 saadmk11 commented Apr 26, 2019

@stsewd Updated the PR. did not touch the constants.py files. I'll work on them in a followup PR. :)

stsewd
stsewd approved these changes Apr 26, 2019
@stsewd stsewd requested a review from Apr 26, 2019
@codecov
Copy link

@codecov codecov bot commented May 9, 2019

Codecov Report

Merging #5632 into master will decrease coverage by <.01%.
The diff coverage is 80.76%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
readthedocs/builds/models.py 82.53% <ø> (-0.06%) ⬇️
readthedocs/core/utils/__init__.py 80.68% <ø> (-0.22%) ⬇️
readthedocs/builds/syncers.py 34.31% <0%> (+1.6%) ⬆️
readthedocs/core/views/serve.py 88.59% <100%> (-0.08%) ⬇️
readthedocs/config/config.py 98.8% <100%> (-0.01%) ⬇️
readthedocs/core/resolver.py 99.08% <100%> (-0.05%) ⬇️
readthedocs/projects/views/base.py 88.52% <100%> (-0.19%) ⬇️
readthedocs/projects/validators.py 77.77% <100%> (-0.35%) ⬇️
readthedocs/notifications/backends.py 94.28% <100%> (-0.16%) ⬇️
readthedocs/projects/views/public.py 75.16% <100%> (-0.17%) ⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 227da1f...152355b. Read the comment docs.

1 similar comment
@codecov
Copy link

@codecov codecov bot commented May 9, 2019

Codecov Report

Merging #5632 into master will decrease coverage by <.01%.
The diff coverage is 80.76%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
readthedocs/builds/models.py 82.53% <ø> (-0.06%) ⬇️
readthedocs/core/utils/__init__.py 80.68% <ø> (-0.22%) ⬇️
readthedocs/builds/syncers.py 34.31% <0%> (+1.6%) ⬆️
readthedocs/core/views/serve.py 88.59% <100%> (-0.08%) ⬇️
readthedocs/config/config.py 98.8% <100%> (-0.01%) ⬇️
readthedocs/core/resolver.py 99.08% <100%> (-0.05%) ⬇️
readthedocs/projects/views/base.py 88.52% <100%> (-0.19%) ⬇️
readthedocs/projects/validators.py 77.77% <100%> (-0.35%) ⬇️
readthedocs/notifications/backends.py 94.28% <100%> (-0.16%) ⬇️
readthedocs/projects/views/public.py 75.16% <100%> (-0.17%) ⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 227da1f...152355b. Read the comment docs.

@saadmk11
Copy link
Member Author

@saadmk11 saadmk11 commented May 9, 2019

This is starting to get conflicts. Can this be merged or further changes required?

stsewd
stsewd approved these changes May 9, 2019
Copy link
Member

@stsewd stsewd left a comment

I would like to have another review from @rtfd/core before merging

humitos
humitos approved these changes May 9, 2019
Copy link
Member

@humitos humitos left a comment

Looks good to me!

@humitos humitos merged commit 1e4a46b into readthedocs:master May 9, 2019
1 check passed
@saadmk11 saadmk11 deleted the cleanup-sync_user-settings branch May 9, 2019
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.

None yet

3 participants