Skip to content

Use default settings for Config object #5055

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.

Sign up for GitHub

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

Jump to bottom
Closed
humitos opened this issue Jan 2, 2019 · 2 comments
Closed

Use default settings for Config object #5055

humitos opened this issue Jan 2, 2019 · 2 comments
Labels
Accepted Accepted issue on our roadmap Improvement Minor improvement to code
Milestone
2.9

Comments

@humitos
Copy link
Member

humitos commented Jan 2, 2019

Our current Config object is using hardcoded values to define some DOCKER settings:

https://github.com/rtfd/readthedocs.org/blob/8466fa45a2fb1b813ef0272b1154e0c98fe62218/readthedocs/config/config.py#L60-L81

This should follow the pattern that we were using at

https://github.com/rtfd/readthedocs.org/blob/8466fa45a2fb1b813ef0272b1154e0c98fe62218/readthedocs/doc_builder/constants.py#L37-L44

This way, we can modify them by setting the values as a setting and also allow to override them from outside (like local_settings.py)

Related to #2140
@humitos humitos added Improvement Minor improvement to code Accepted Accepted issue on our roadmap labels Jan 2, 2019
@humitos humitos added this to the 2.9 milestone Jan 2, 2019
@humitos
Copy link
Member Author

humitos commented Jan 2, 2019

The same happens at

https://github.com/rtfd/readthedocs.org/blob/8466fa45a2fb1b813ef0272b1154e0c98fe62218/readthedocs/config/config.py#L278-L279

@humitos
Copy link
Member Author

humitos commented Jan 2, 2019

Also, modifying PYTHON_SUPPORTED_VERSIONS has no effect because it then uses

https://github.com/rtfd/readthedocs.org/blob/8466fa45a2fb1b813ef0272b1154e0c98fe62218/readthedocs/config/config.py#L283-L289

and returns at line 286 where the class variable was not used.

I found this flow kind of confusing and it's hard to know where we need to touch to get a new value supported.

It would be good if we can centralize this on only one setting/variable.

@humitos humitos mentioned this issue Jan 2, 2019
Merged
@dojutsu-user dojutsu-user mentioned this issue Jan 2, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Accepted Accepted issue on our roadmap Improvement Minor improvement to code
Projects
None yet
Milestone
2.9
Development

No branches or pull requests
1 participant
@humitos