Skip to content

fix(docs): add sphinx dependencies to Pipfile dev-packages#1214

Merged
Strift merged 2 commits intomeilisearch:mainfrom
vibeyclaw:fix/docs-sphinx-dependencies
Mar 3, 2026
Merged

fix(docs): add sphinx dependencies to Pipfile dev-packages#1214
Strift merged 2 commits intomeilisearch:mainfrom
vibeyclaw:fix/docs-sphinx-dependencies

Conversation

@vibeyclaw
Copy link
Contributor

@vibeyclaw vibeyclaw commented Feb 28, 2026

Summary

Fixes #1193

The Documentation workflow was failing at the Install dependencies step because sphinx and sphinx-rtd-theme were installed via separate pipenv install commands after pipenv install --dev, which can cause dependency resolution conflicts.

Changes

  • Added sphinx and sphinx-rtd-theme to [dev-packages] in Pipfile
  • Removed the redundant pipenv install sphinx and pipenv install sphinx_rtd_theme lines from the workflow (they are now installed as part of pipenv install --dev)

This consolidates all dev dependencies into a single install step, allowing pipenv to resolve them together without conflicts.

Summary by CodeRabbit

  • Chores
    • Centralized and expanded development dependencies in the Pipfile; CI no longer installs certain documentation packages explicitly.
  • Style
    • Minor internal message-formatting cleanups for error handling (no behavioral change).

Move sphinx and sphinx-rtd-theme from ad-hoc installs in the workflow
to Pipfile dev-packages. This ensures dependency resolution is handled
by pipenv correctly, fixing the failing Documentation workflow.

Fixes meilisearch#1193
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 28, 2026

📝 Walkthrough

Walkthrough

Removed explicit Sphinx installs from the documentation workflow, added multiple development dependencies to Pipfile under [dev-packages], and reformatted three exception-raising constructions in meilisearch/_httprequests.py without changing behavior.

Changes

Cohort / File(s) Summary
Workflow Configuration
.github/workflows/documentation.yml
Deleted two pipenv run pip install lines that explicitly installed sphinx and sphinx_rtd_theme in the "Install dependencies" step.
Dependency Manifest
Pipfile
Added many dev-packages: sphinx, sphinx-rtd-theme, pdoc3, pytest-ordering, tox, tox-pipenv, types-requests, black, isort, exceptiongroup (py<3.11 marker), tomli (py<3.11 marker), wrapt (py<3.11 marker), dill, pytest-cov, plus extras; centralizes doc/test/lint deps.
HTTP Request Handling
meilisearch/_httprequests.py
Reformatted three MeilisearchCommunicationError constructions for InvalidSchema cases (in send_request and two locations in post_stream) to pass a single multiline f-string as the error message; semantics unchanged.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I hopped through Pipfile, tidy and bright,
Pulled Sphinx from workflow into dev-package light,
Messages trimmed neatly in request-error song,
Docs and errors now humming along. 🥕✨

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The PR includes formatting changes to error handling in meilisearch/_httprequests.py that are unrelated to the documentation build fix objective stated in issue #1193. Revert the formatting changes in meilisearch/_httprequests.py (InvalidSchema error handling) as they are out of scope and should be addressed in a separate PR.
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the main change: adding sphinx dependencies to Pipfile dev-packages to fix documentation workflow.
Linked Issues check ✅ Passed The PR successfully addresses issue #1193 by adding sphinx and sphinx-rtd-theme to Pipfile dev-packages and removing redundant installation steps from the Documentation workflow.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
meilisearch/_httprequests.py (1)

86-89: Consider centralizing the invalid-URL message builder to avoid duplicated multiline strings.

Line 86 and Line 204 duplicate the same triple-quoted payload. Extracting this into a helper keeps both paths consistent and avoids indentation/newline artifacts in final error text.

♻️ Suggested refactor
 class HttpRequests:
+    `@staticmethod`
+    def _invalid_url_message(url: str) -> str:
+        return (
+            f"Invalid URL {url}, no scheme/protocol supplied. "
+            f"Did you mean https://{url}?"
+        )
+
     def send_request(
         self,
         http_method: Callable,
@@
         except requests.exceptions.InvalidSchema as err:
             if "://" not in self.config.url:
-                raise MeilisearchCommunicationError(f"""
-                    Invalid URL {self.config.url}, no scheme/protocol supplied.
-                    Did you mean https://{self.config.url}?
-                    """) from err
+                raise MeilisearchCommunicationError(
+                    self._invalid_url_message(self.config.url)
+                ) from err
 
             raise MeilisearchCommunicationError(str(err)) from err
@@
         except requests.exceptions.InvalidSchema as err:
             if "://" not in self.config.url:
-                raise MeilisearchCommunicationError(f"""
-                    Invalid URL {self.config.url}, no scheme/protocol supplied.
-                    Did you mean https://{self.config.url}?
-                    """) from err
+                raise MeilisearchCommunicationError(
+                    self._invalid_url_message(self.config.url)
+                ) from err
 
             raise MeilisearchCommunicationError(str(err)) from err

Also applies to: 204-207

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@meilisearch/_httprequests.py` around lines 86 - 89, Extract the duplicated
triple-quoted invalid-URL payload into a single helper (e.g.,
_invalid_url_message or _invalid_url_error_message) that accepts the URL and
returns a single formatted string (no leading/trailing newlines/indentation),
then replace the two inline multi-line raises of MeilisearchCommunicationError
with raises that call this helper (references: MeilisearchCommunicationError and
the two raise sites in meilisearch/_httprequests.py). Ensure the helper is
module-level or a static/private method on the same class so both locations use
it and the final error text is identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@meilisearch/_httprequests.py`:
- Around line 86-89: Extract the duplicated triple-quoted invalid-URL payload
into a single helper (e.g., _invalid_url_message or _invalid_url_error_message)
that accepts the URL and returns a single formatted string (no leading/trailing
newlines/indentation), then replace the two inline multi-line raises of
MeilisearchCommunicationError with raises that call this helper (references:
MeilisearchCommunicationError and the two raise sites in
meilisearch/_httprequests.py). Ensure the helper is module-level or a
static/private method on the same class so both locations use it and the final
error text is identical.

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bc38ef3 and eeac5f5.

📒 Files selected for processing (1)
  • meilisearch/_httprequests.py

@Strift Strift added the documentation Improvements or additions to documentation label Mar 3, 2026
@Strift Strift added this pull request to the merge queue Mar 3, 2026
@Strift Strift added the maintenance Anything related to maintenance (CI, tests, refactoring...) label Mar 3, 2026
Merged via the queue into meilisearch:main with commit c976466 Mar 3, 2026
11 checks passed
@Strift
Copy link
Contributor

Strift commented Mar 3, 2026

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation maintenance Anything related to maintenance (CI, tests, refactoring...)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix documentation build

2 participants