fix(docs): add sphinx dependencies to Pipfile dev-packages#1214
fix(docs): add sphinx dependencies to Pipfile dev-packages#1214Strift merged 2 commits intomeilisearch:mainfrom
Conversation
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
📝 WalkthroughWalkthroughRemoved explicit Sphinx installs from the documentation workflow, added multiple development dependencies to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
🧹 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 errAlso 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.
|
Great work! It worked: https://github.com/meilisearch/meilisearch-python/actions/runs/22607973806 🙌 |
Summary
Fixes #1193
The Documentation workflow was failing at the Install dependencies step because
sphinxandsphinx-rtd-themewere installed via separatepipenv installcommands afterpipenv install --dev, which can cause dependency resolution conflicts.Changes
sphinxandsphinx-rtd-themeto[dev-packages]inPipfilepipenv install sphinxandpipenv install sphinx_rtd_themelines from the workflow (they are now installed as part ofpipenv install --dev)This consolidates all dev dependencies into a single install step, allowing pipenv to resolve them together without conflicts.
Summary by CodeRabbit