👌 Improve and test github needservice directive

pyproject.toml

@@ -48,7 +48,6 @@ matplotlib = { version = ">=3.3.0", optional = true }
 pytest = { version = "^7", optional = true }
 pytest-cov = { version = "^4", optional = true }
 lxml = { version = "^4.6.5", optional = true }
-requests-mock = { version = ">=1.9.3", optional = true }
 responses = { version = "^0.22.0", optional = true }
 sphinxcontrib-plantuml = { version = "^0", optional = true }
 syrupy = { version = "^3", optional = true }
@@ -76,7 +75,6 @@ test = [
   "pytest-cov",
   "syrupy",
   "sphinxcontrib-plantuml",
-  "requests-mock",
   "lxml",
   "responses",
   "pytest-xprocess"

sphinx_needs/directives/needservice.py

@@ -11,9 +11,9 @@
 
 from sphinx_needs.api import add_need
 from sphinx_needs.config import NeedsSphinxConfig
+from sphinx_needs.data import SphinxNeedsData
 from sphinx_needs.directives.need import NeedDirective
 from sphinx_needs.logging import get_logger
-from sphinx_needs.services.base import BaseService
 from sphinx_needs.utils import add_doc
 
 
@@ -67,15 +67,15 @@ def run(self) -> Sequence[nodes.Node]:
         needs_config = NeedsSphinxConfig(self.config)
         need_types = needs_config.types
         all_data = needs_config.service_all_data
-        needs_services: dict[str, BaseService] = getattr(app, "needs_services", {})
+        needs_services = SphinxNeedsData(self.env).get_or_create_services()
 
         service_name = self.arguments[0]
         service = needs_services.get(service_name)
         assert service is not None
         section = []
 
         if "debug" not in self.options:
-            service_data = service.request(self.options)
+            service_data = service.request_from_directive(self)
             for datum in service_data:
                 options = {}
 

sphinx_needs/services/base.py

@@ -2,6 +2,8 @@
 
 from typing import Any, ClassVar
 
+from sphinx.util.docutils import SphinxDirective
+
 from sphinx_needs.logging import get_logger
 
 
@@ -11,8 +13,17 @@ class BaseService:
     def __init__(self, *args: Any, **kwargs: Any) -> None:
         self.log = get_logger(__name__)
 
-    def request(self, *args: Any, **kwargs: Any) -> Any:
+    # note the public API originally just had the `request` method that parsed options as the first argument,
+    # and so to not break existing subclasses, we keep that method signature,
+    # then also introduce `request_from_directive`, which is what is now called by the directive
+
+    def request(self, *args: Any, **kwargs: Any) -> list[dict[str, Any]]:
         raise NotImplementedError("Must be implemented by the service!")
 
+    def request_from_directive(
+        self, directive: SphinxDirective, /
+    ) -> list[dict[str, Any]]:
+        return self.request(directive.options)
+
     def debug(self, *args: Any, **kwargs: Any) -> Any:
         raise NotImplementedError("Must be implemented by the service!")

sphinx_needs/services/github.py

@@ -9,6 +9,8 @@
 
 import requests
 from sphinx.application import Sphinx
+from sphinx.util.docutils import SphinxDirective
+from sphinx.util.logging import getLogger
 
 from sphinx_needs.api import add_need_type
 from sphinx_needs.api.exceptions import NeedsApiConfigException
@@ -23,6 +25,8 @@
     GITHUB_LAYOUT,
 )
 
+LOGGER = getLogger(__name__)
+
 
 class GithubService(BaseService):
     options = (
@@ -39,6 +43,7 @@
         self.url = self.config.get("url", "https://api.github.com/")
         if not self.url.endswith("/"):
             self.url = f"{self.url}/"
+        self.retry_delay = self.config.get("retry_delay", 61)
         self.max_amount = self.config.get("max_amount", -1)
         self.max_content_lines = self.config.get("max_content_lines", -1)
         self.id_prefix = self.config.get("id_prefix", "GITHUB_")
@@ -109,7 +114,7 @@
                     single_type = "commits"
                 url = self.url + f"repos/{owner}/{repo}/{single_type}/{number}"
             except IndexError:
-                raise NeedGithubServiceException(
+                raise _SendException(
                     'Single option ot valid, must follow "owner/repo/number"'
                 )
 
@@ -140,25 +145,25 @@
                 resp_limit = requests.get(self.url + "rate_limit", auth=auth)
                 extra_info = resp_limit.json()
                 self.log.info(
-                    "GitHub: API rate limit exceeded. We need to wait 60 secs..."
+                    f"GitHub: API rate limit exceeded. trying again in {self.retry_delay} seconds..."
                 )
                 self.log.info(extra_info)
-                time.sleep(61)
+                time.sleep(self.retry_delay)
                 resp = requests.get(url, params=params, auth=auth, headers=headers)
                 if resp.status_code > 299:
                     if "rate limit" in resp.json()["message"]:
-                        raise NeedGithubServiceException(
+                        raise _SendException(
                             "GitHub: API rate limit exceeded (twice). Stop here."
                         )
                     else:
-                        raise NeedGithubServiceException(
+                        raise _SendException(
                             "Github service error during request.\n"
                             f"Status code: {resp.status_code}\n"
                             f"Error: {resp.text}\n"
                             f"{extra_info}"
                         )
             else:
-                raise NeedGithubServiceException(
+                raise _SendException(
                     "Github service error during request.\n"
                     f"Status code: {resp.status_code}\n"
                     f"Error: {resp.text}\n"
@@ -169,48 +174,63 @@
             return {"items": [resp.json()]}
         return resp.json()  # type: ignore
 
-    def request(self, options: dict[str, Any] | None = None) -> list[dict[str, Any]]:
-        if options is None:
-            options = {}
+    def request_from_directive(
+        self, directive: SphinxDirective, /
+    ) -> list[dict[str, Any]]:
         self.log.debug(f"Requesting data for service {self.name}")
+        options = directive.options
 
         if "query" not in options and "specific" not in options:
-            raise NeedGithubServiceException(
-                '"query" or "specific" missing as option for github service.'
+            create_warning(
+                directive,
+                '"query" or "specific" missing as option for github service.',
             )
-        elif "query" in options and "specific" in options:
-            raise NeedGithubServiceException(
-                'Only "query" or "specific" allowed for github service. Not both!'
+            return []
+
+        if "query" in options and "specific" in options:
+            create_warning(
+                directive,
+                'Only "query" or "specific" allowed for github service. Not both!',
             )
-        elif "query" in options:
+            return []
+
+        if "query" in options:
             query = options["query"]
             specific = False
         else:
             query = options["specific"]
             specific = True
 
-        response = self._send(query, options, specific=specific)
+        try:
+            response = self._send(query, options, specific=specific)
+        except _SendException as e:
+            create_warning(directive, str(e))
+            return []
         if "items" not in response:
             if "errors" in response:
-                raise NeedGithubServiceException(
+                create_warning(
+                    directive,
                     "GitHub service query error: {}\n" "Used query: {}".format(
                         response["errors"][0]["message"], query
-                    )
+                    ),
                 )
+                return []
             else:
-                raise NeedGithubServiceException("Github service: Unknown error.")
+                create_warning(directive, "Github service: Unknown error")
+                return []
 
         if self.gh_type == "issue" or self.gh_type == "pr":
-            data = self.prepare_issue_data(response["items"], options)
+            data = self.prepare_issue_data(response["items"], directive)
         elif self.gh_type == "commit":
-            data = self.prepare_commit_data(response["items"], options)
+            data = self.prepare_commit_data(response["items"], directive)
         else:
-            raise NeedGithubServiceException("Github service failed. Wrong gh_type...")
+            create_warning(directive, "Github service failed. Wrong gh_type...")
+            return []
 
         return data
 
     def prepare_issue_data(
-        self, items: list[dict[str, Any]], options: dict[str, Any]
+        self, items: list[dict[str, Any]], directive: SphinxDirective
     ) -> list[dict[str, Any]]:
         data = []
         for item in items:
@@ -231,9 +251,11 @@
 
             content = "\n\n   ".join(content_lines)
             # Reduce content length, if requested by config
-            if self.max_content_lines > 0:
+            if (self.max_content_lines > 0) or (
+                "max_content_lines" in directive.options
+            ):
                 max_lines = int(
-                    options.get("max_content_lines", self.max_content_lines)
+                    directive.options.get("max_content_lines", self.max_content_lines)
                 )
                 content_lines = content.splitlines()
                 if len(content_lines) > max_lines:
@@ -245,21 +267,21 @@
             # everything in a safe code-block
             content = ".. code-block:: text\n\n   " + content
 
-            prefix = options.get("id_prefix", self.id_prefix)
+            prefix = directive.options.get("id_prefix", self.id_prefix)
             need_id = prefix + str(item["number"])
-            given_tags = options.get("tags", False)
+            given_tags = directive.options.get("tags", False)
             github_tags = ",".join([x["name"] for x in item["labels"]])
             if given_tags:
                 tags = str(given_tags) + ", " + str(github_tags)
             else:
                 tags = github_tags
 
-            avatar_file_path = self._get_avatar(item["user"]["avatar_url"])
+            avatar_file_path = self._get_avatar(item["user"]["avatar_url"], directive)
 
             element_data = {
                 "service": self.name,
-                "type": options.get("type", self.need_type),
-                "layout": options.get("layout", self.layout),
+                "type": directive.options.get("type", self.need_type),
+                "layout": directive.options.get("layout", self.layout),
                 "id": need_id,
                 "title": item["title"],
                 "content": content,
@@ -272,23 +294,23 @@
                 "updated_at": item["updated_at"],
                 "closed_at": item["closed_at"],
             }
-            self._add_given_options(options, element_data)
+            self._add_given_options(directive.options, element_data)
             data.append(element_data)
 
         return data
 
     def prepare_commit_data(
-        self, items: list[dict[str, Any]], options: dict[str, Any]
+        self, items: list[dict[str, Any]], directive: SphinxDirective
     ) -> list[dict[str, Any]]:
         data = []
 
         for item in items:
-            avatar_file_path = self._get_avatar(item["author"]["avatar_url"])
+            avatar_file_path = self._get_avatar(item["author"]["avatar_url"], directive)
 
             element_data = {
                 "service": self.name,
-                "type": options.get("type", self.need_type),
-                "layout": options.get("layout", self.layout),
+                "type": directive.options.get("type", self.need_type),
+                "layout": directive.options.get("layout", self.layout),
                 "id": self.id_prefix + item["sha"][:6],
                 "title": item["commit"]["message"].split("\n")[0][
                     :60
@@ -299,18 +321,13 @@
                 "avatar": avatar_file_path,
                 "created_at": item["commit"]["author"]["date"],
             }
-            self._add_given_options(options, element_data)
+            self._add_given_options(directive.options, element_data)
             data.append(element_data)
 
         return data
 
-    def _get_avatar(self, avatar_url: str) -> str:
-        """
-        Download and store avatar image
-
-        :param avatar_url:
-        :return:
-        """
+    def _get_avatar(self, avatar_url: str, directive: SphinxDirective) -> str:
+        """Download and store avatar image"""
         url_parsed = urlparse(avatar_url)
         filename = os.path.basename(url_parsed.path) + ".png"
         path = os.path.join(self.app.srcdir, self.download_folder)
@@ -334,22 +351,22 @@
                     with open(avatar_file_path, "wb") as f:
                         f.write(response.content)
                 elif response.status_code == 302:
-                    self.log.warning(
+                    create_warning(
+                        directive,
                         f"GitHub service {self.name} could not download avatar image "
                         f"from {avatar_url}.\n"
                         f"    Status code: {response.status_code}\n"
                         "    Reason: Looks like the authentication provider tries to redirect you."
                         " This is not supported and is a common problem, "
-                        "if you use GitHub Enterprise. [needs]",
-                        type="needs",
+                        "if you use GitHub Enterprise.",
                     )
                     avatar_file_path = default_avatar_file_path
                 else:
-                    self.log.warning(
+                    create_warning(
+                        directive,
                         f"GitHub service {self.name} could not download avatar image "
                         f"from {avatar_url}.\n"
-                        f"    Status code: {response.status_code} [needs]",
-                        type="needs",
+                        f"    Status code: {response.status_code}",
                     )
                     avatar_file_path = default_avatar_file_path
         else:
@@ -373,5 +390,14 @@
                 element_data[key] = value
 
 
-class NeedGithubServiceException(BaseException):
+class _SendException(Exception):
     pass
+
+
+def create_warning(directive: SphinxDirective, message: str) -> None:
+    LOGGER.warning(
+        message + " [needs.github]",
+        type="needs",
+        subtype="github",
+        location=directive.get_location(),
+    )

sphinx_needs/services/open_needs.py

@@ -7,6 +7,7 @@
 import requests
 from jinja2 import Template
 from sphinx.application import Sphinx
+from sphinx.util.docutils import SphinxDirective
 
 from sphinx_needs.config import NeedsSphinxConfig
 from sphinx_needs.utils import dict_get, jinja_parse
@@ -213,10 +214,12 @@ def _extract_data(
             need_data.append(finale_data)
         return need_data
 
-    def request(self, options: Any = None, *args: Any, **kwargs: Any) -> Any:
+    def request_from_directive(
+        self, directive: SphinxDirective, /
+    ) -> list[dict[str, Any]]:
         self.log.info(f"Requesting data for service {self.name}")
         self._oauthorization()  # Get authorization token
-        params = self._prepare_request(options)
+        params = self._prepare_request(directive.options)
 
         request_params = {
             "url": params["url"],
@@ -230,7 +233,7 @@ def request(self, options: Any = None, *args: Any, **kwargs: Any) -> Any:
             if "description" not in datum or datum["description"] is None:
                 datum["description"] = ""
 
-        need_data = self._extract_data(data, options)
+        need_data = self._extract_data(data, directive.options)
 
         return need_data