Add sphinx factory class #3656
Add sphinx factory class #3656
Conversation
|
I know this is still WIP. But this is very large refactoring. So I'd like to merge this into master branch and improve step by step. |
sphinx/factory.py
Outdated
| } # type: Dict[unicode, unicode] | ||
|
|
||
|
|
||
| class SphinxFactory(object): |
There was a problem hiding this comment.
I feel the name of Factory is not good.
IMO, this factory class seems having 2 responsibilities; factory and registry.
- registry for builders, domains, source_parsers and translators.
- adding object to registry.
- getting object from registry.
- having loading extension mechanism.
- creating instance of builders, domains and translators.
I think 4 and 5 are the responsibility of factory, but 1,2, 3 are registry.
There was a problem hiding this comment.
Indeed, it is not good naming.
question: do you mean splitting the feature to two classes?
There was a problem hiding this comment.
As a result of discussion, I finally renamed it to SphinxComponentRegistry.
sphinx/builders/__init__.py
Outdated
| def get_translator_class(self, *args): | ||
| # type: (Any) -> nodes.NodeVisitor | ||
| """Return a class of translator.""" | ||
| return self.app.factory.get_translator_class(self) |
There was a problem hiding this comment.
In this case, it seems that app.factoryworks as a registry for internal mechanism.
So every internal mechanism should access to app.registry instead of app.factory if my understanding is correct.
It means that app.factory has two responsibility; factory and registry.
BTW (not important) I feel that this reference is deep. IMHO, principle of least knowledge (A.K.A. The Law of Demeter) is good practice.
|
Merged. Thank you for reviewing! |
As a refactoring, I'd like to introduce a factory class of sphinx components:
This is a first step of refactoring. I will continue to improve this to whole of components.
I believe this brings our application class simple.
At present, the factory class is only used to make application class simple. Hopefully, I'd like to give the factory object to
setup()function of each extensions. I think it is much simpler.