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

Refactor BuildCommand and move outside doc_builder/ #3541

Closed
humitos opened this issue Jan 23, 2018 · 5 comments
Closed

Refactor BuildCommand and move outside doc_builder/ #3541

humitos opened this issue Jan 23, 2018 · 5 comments
Labels
Accepted Accepted issue on our roadmap Improvement Minor improvement to code
Milestone

Comments

@humitos
Copy link
Member

humitos commented Jan 23, 2018

Once #3520 got merged, we will end up with a couple of name that are inconsistent: https://github.com/rtfd/readthedocs.org/pull/3520/files/7f6c098983acb4bb3afc90e5c780af7dae9ec9d8#r163198277

In this case it should be named just Command and moved out doc_builder/. Besides a new BuildCommand should exist that handles all the interaction with the Build itself and saving the command in the database.

The idea is to find out more names like that one and refactor.

@humitos humitos added the Improvement Minor improvement to code label Jan 23, 2018
@agjohnson agjohnson added this to the Refactoring milestone Sep 17, 2018
@stale
Copy link

stale bot commented Jan 10, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Status: stale Issue will be considered inactive soon label Jan 10, 2019
@stsewd stsewd added Accepted Accepted issue on our roadmap and removed Status: stale Issue will be considered inactive soon labels Jan 10, 2019
@dojutsu-user
Copy link
Member

@humitos
I request you to please update the issue description as the link provided is not working anymore.
I am interested in sending a PR for this.

@humitos
Copy link
Member Author

humitos commented Apr 8, 2019

@dojutsu-user I don't remember exactly what I wanted to do, but it seems that we have some classes in doc_builder/environments.py that can be refactored and move it out from that directory.

I think there are some class names that don't really make sense because they have a Build prefix but they don't handle a build, or similar.

I suggest you to read the PR linked and check if you find our a good refactor that can be applied to those classes. Otherwise, we could probably close the issue if you find out that it's obsolete and there are no good refactor here.

@dojutsu-user
Copy link
Member

@humitos
I did go through the attached PR and there are some small refactors suggested there.
I am working on it.
Will make a PR soon.

@humitos
Copy link
Member Author

humitos commented Mar 15, 2022

A similar refactor was already done in #8815, so we can close this issue.

@humitos humitos closed this as completed Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Accepted issue on our roadmap Improvement Minor improvement to code
Projects
None yet
Development

No branches or pull requests

4 participants