Services - please help!

[danieleades]

I have lots of questions about the 'services' module!

i think there's lots of room for improvement.

• what is the purpose of the BaseService class? It looks like it's meant to act as an interface, but it's function signatures are completely general, and don't match the derived classes, so i don't see that it's adding much value?
• do you think it might be better to strongly-type the options that each service requires?
• the __init__ method of the github service is working pretty hard here. it doesn't feel like the responsibility of checking the presence of config values, registering need_types, and registering layouts should live here.

i've made a quick sketch of an alternative approach-

base class

from sphinxcontrib.needs.logging import getLogger
from abc import ABC, abstractmethod, abstractstaticmethod
from typing import TypeVar, Generic, Type

Config = TypeVar('Config')


class BaseService(ABC, Generic[Config]):

    def __init__(self):
        self.log = getLogger(__name__)

    @abstractmethod
    def request(self, *args, **kwargs):
        pass

    @abstractstaticmethod
    def options() -> Type[Config]:
        pass

github service

@dataclass
class GithubServiceConfig:
    username: str
    token: str
    url: str = "https://api.github.com/"
    max_amount: int = -1
    max_content_lines: int = -1
    id_prefix: str = "GITHUB_"
    layout: str = "github"
    download_avatars: bool = True
    download_folder: Path = Path("github_images")


class GithubService(BaseService):
    def __init__(self, app: Sphinx, name: str, config: GithubServiceConfig):
        super().__init__()
        self.app = app
        self.name = name
        self.config = config

    @staticmethod
    def options() -> Type[GithubServiceConfig]:
        return GithubServiceConfig

service_manager

class ServiceManager:
    def __init__(self, app):
        self.app = app

        self.log = getLogger(__name__)
        self.services = {}

    def register(self, name, clazz: BaseService):
        raw_config = self.app.config.needs_extra_options.get(name, {})
        config = clazz.options()(**raw_config)
        self.services[name] = clazz(self.app, name, config)

what do you think of this as a possible direction?

[danieleades]

here's a slightly more complete version

note that the service is not registering any needs. would be better to return these as static variables and let the manager register them (i've left this out so far). The service shouldn't need to know anything about Sphinx.

service base

from sphinxcontrib.needs.logging import getLogger
from abc import ABC, abstractmethod
from typing import TypeVar, Generic, Type

Config = TypeVar("Config")


class BaseService(ABC, Generic[Config]):
    def __init__(self, config: Config):
        self.log = getLogger(__name__)
        self.config = config

    @abstractmethod
    def request(self, *args, **kwargs):
        pass

    @staticmethod
    @abstractmethod
    def options() -> Type[Config]:
        pass

    @staticmethod
    @abstractmethod
    def name() -> str:
        pass

github service

@dataclass
class GithubServiceConfig:
    username: str
    token: str
    url: str = "https://api.github.com/"
    max_amount: int = -1
    max_content_lines: int = -1
    id_prefix: str = "GITHUB_"
    layout: str = "github"
    download_avatars: bool = True
    download_folder: Path = Path("github_images")


class GithubService(BaseService):

    @staticmethod
    def options() -> Type[GithubServiceConfig]:
        return GithubServiceConfig

    @staticmethod
    def name() -> str:
        return "github"

service manager

from sphinxcontrib.needs.logging import getLogger
from sphinxcontrib.needs.services.base import BaseService
from typing import Dict, Type, Any

Options = Dict[str, Any]


class ServiceManager:
    def __init__(self, extra_options: Dict[str, Options]):
        self.log = getLogger(__name__)
        self.extra_options = extra_options
        self.services: Dict[str, BaseService] = {}

    def register(self, clazz: Type[BaseService]):
        name = clazz.name()
        raw_config = self.extra_options.get(name, {})
        config = clazz.options().__init__(**raw_config)

        self.services[name] = clazz(config)

it's a bit tidier, no?

[danieleades]

Also, you may not need to reinvent the wheel for the GitHub API client

• https://github.com/PyGithub/PyGithub
• https://ghapi.fast.ai/

[danwos]

I know, but sphinx-needs already has so many deps, which are only used for specific directives/use cases.
And I only needed to fetch issues, so not so much code here.
But yeah, it may be more stable and reliable to use a tested, well maintained lib here.

Starting with #222 sphinx-needs may introduce optional dependencies.
For sure also an option for this case.

[danieleades]

i agree with that- i think the 'services' are a good candidate for an optional feature. or possibly a separate extension. (a sphinx-extensions-extension, if you will!)

[danwos]

* what is the purpose of the BaseService class? It looks like it's meant to act as an interface, but it's function signatures are completely general, and don't match the derived classes, so i don't see that it's adding much value?

Yes, BaseService class does not contain much (only request func)
It's more a placeholder for common functions for the future.
I just want to be sure that all services already inherit from a common class.

* do you think it might be better to strongly-type the options that each `service` requires?

Not 100% what you mean.
Sphinx options are always from type string.
So any other type would mean we need to transform it.
Possible, but I think for this the service itself can be responsible.

* the `__init__` method of the github service is working pretty hard here. it doesn't feel like the responsibility of checking the presence of config values, registering need_types, and registering layouts should live here.

Your dataclass approach looks promising. We could put most of the "registration" stuff to an other function, which is part of BaseService class and gets called by the manager after service initialization.
Or the BaseService class also provides functions to register need_types and layouts. Benefit: Common error handling and logging for all services.

from sphinxcontrib.needs.logging import getLogger
from abc import ABC, abstractmethod
from typing import TypeVar, Generic, Type

Config = TypeVar("Config")


class BaseService(ABC, Generic[Config]):
    def __init__(self, config: Config):
        self.log = getLogger(__name__)
        self.config = config

    @abstractmethod
    def request(self, *args, **kwargs):
        pass

    @staticmethod
    @abstractmethod
    def options() -> Type[Config]:
        pass

    @staticmethod
    @abstractmethod
    def name() -> str:
        pass
    
    def register_need_type(name):
        try:
            add_need_type(name, ...)
       except:
            ....
            
    def register_layout(name, layout):
        ....          

Or even better the "register" functions are part of the ServiceManager and each service gets access to it during __init__(app, service_manager, ...)

[danieleades]

Sphinx options are always from type string

are you sure this is true? - https://www.sphinx-doc.org/en/master/extdev/appapi.html#sphinx.application.Sphinx.add_config_value

the add_config_value takes a types parameter. I'm not entirely sure how this behaves, i've not looked at the source code. Might look into that

[danieleades]

i'll try and do a proper write-up of all this in a pull request. Note that using dataclasses requires python 3.7 minimum. you can get the same functionality using the pydantic library. You could also use plain classes, but it would be a lot more verbose.

reasons i'm suggesting this pattern over the current implementation

• derived service classes shouldn't be responsible for registering themselves. They shouldn't need to know or care what a Sphinx Application is, or how the registration works. Much better from the perspective of encapsulation to do that in the calling code.
• use concrete types to specify the 'options' required by the service classes. probably worth doing something similar for the request method. Having a base class with a def request(self, *args, **kwargs) adds no value, since this method can take literally any arguments.
• composition over inheritance! registering services explicitly in a ServiceManager is preferable to having this done implicitly in a base class. I think inheritance in Python is fine for polymorphism, but it's almost always an antipattern for code reuse.

anyway, i'm talking in hypotheticals here. If i get some time i'll put together a pull request and we discuss something concrete. I'm just hand-waving, currently.