Skip to content

Conversation

Strift
Copy link
Collaborator

@Strift Strift commented Sep 12, 2025

Pull Request

Preliminary work for #761

  • Update CI to run against v1.x branch
  • Update contributors in composer.json 🥳
  • Fix phpstan warnings (not sure why this was triggered 🤷‍♂️)

PR checklist

Please check if your PR fulfills the following requirements:

  • Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
  • Have you read the contributing guidelines?
  • Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

Summary by CodeRabbit

  • Bug Fixes

    • Error message formatting improved across exceptions: non-empty messages (including “0”) are now displayed, while empty messages are omitted, improving clarity in logs without altering runtime behavior.
  • Chores

    • CI updated to run tests and draft releases on pushes to v1.x branches in addition to main.
    • Project metadata updated with additional authors; script formatting adjusted with no functional changes.

@Strift Strift added the maintenance Anything related to maintenance (CI, tests, refactoring...) label Sep 12, 2025
Copy link

coderabbitai bot commented Sep 12, 2025

Walkthrough

Adds v1.x to GitHub workflow branch triggers, updates composer authors and script formatting, and changes exception __toString message checks to use non-empty string comparisons across three exception classes.

Changes

Cohort / File(s) Summary
CI workflows: add v1.x push triggers
\.github/workflows/release-drafter.yml, \.github/workflows/tests.yml
Extend on.push.branches to include v1.x so Release Drafter and CI tests run on v1.x pushes; no other workflow logic changes.
Composer metadata and script formatting
composer.json
Expand authors list from 1 to 4 with updated names/emails; reformat scripts.test array without changing the command.
Exception __toString non-empty message checks
src/Exceptions/ApiException.php, src/Exceptions/InvalidResponseBodyException.php, src/Exceptions/TimeOutException.php
Replace null/truthy checks with strict non-empty string checks ('' !== $this->message) to control inclusion of Message in string output.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I thump my paws on v1.x trails,
CI now listens where the branch prevails.
Authors multiply, carrots to share,
Exceptions speak only when words are there.
Hop, hop—neat tweaks in tidy rows,
Release notes ready—off it goes! 🥕✨

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.

Pre-merge checks (2 passed, 1 warning)

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title "Update repository config for v2" is concise and accurately reflects the repository/config-focused changes (e.g., workflow updates adding v1.x triggers), but the PR also includes unrelated edits (composer authors and exception __toString tweaks) that the title does not explicitly mention.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch chore/update-config-v2

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.

@Strift Strift force-pushed the chore/update-config-v2 branch from d44deaa to 5cd3792 Compare September 12, 2025 09:10
@Strift Strift marked this pull request as ready for review September 12, 2025 09:13
Copy link

@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.

Actionable comments posted: 0

🧹 Nitpick comments (4)
.github/workflows/tests.yml (1)

75-75: Avoid using the drifting Docker tag 'latest' for CI stability

Pin the Meilisearch service image to a major or exact version to reduce flaky failures when the upstream image changes.

Apply:

-        image: getmeili/meilisearch:latest
+        image: getmeili/meilisearch:v1
# or pin an exact version once decided, e.g. v1.11.0
src/Exceptions/InvalidResponseBodyException.php (1)

28-30: More precise non-empty check for message — good; consider trimming whitespace

Current change preserves messages like "0". To also skip whitespace-only messages:

-        if ('' !== $this->message) {
+        if ('' !== trim((string) $this->message)) {
             $base .= ' - Message: '.$this->message;
         }
src/Exceptions/TimeOutException.php (1)

23-27: Consistent style with other exceptions and skip whitespace-only messages

Minor clean-up: mirror the pattern used elsewhere and reduce branching.

-        if ('' !== $this->message) {
-            return $base.' - Message: '.$this->message;
-        } else {
-            return $base;
-        }
+        if ('' !== trim((string) $this->message)) {
+            $base .= ' - Message: '.$this->message;
+        }
+        return $base;
src/Exceptions/ApiException.php (1)

36-38: Non-empty check — good; consider trimming to ignore whitespace-only values

Keeps "0" while avoiding null-related pitfalls. Optional trim for parity with other suggestions:

-        if ('' !== $this->message) {
+        if ('' !== trim((string) $this->message)) {
             $base .= ' - Message: '.$this->message;
         }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 04f7e54 and d91882a.

📒 Files selected for processing (6)
  • .github/workflows/release-drafter.yml (1 hunks)
  • .github/workflows/tests.yml (1 hunks)
  • composer.json (2 hunks)
  • src/Exceptions/ApiException.php (1 hunks)
  • src/Exceptions/InvalidResponseBodyException.php (1 hunks)
  • src/Exceptions/TimeOutException.php (1 hunks)
🔇 Additional comments (4)
.github/workflows/tests.yml (1)

12-12: v1.x push trigger — LGTM

This will run CI on maintenance updates to v1.x. Matches the PR’s intent.

.github/workflows/release-drafter.yml (1)

7-7: Add v1.x to Release Drafter triggers — LGTM

Ensure RELEASE_DRAFTER_TOKEN is available to workflows on the v1.x branch protection settings.

composer.json (2)

16-30: Authors metadata update — LGTM

Accurate credits; no functional impact.


73-75: Test script reformat — LGTM

Pure formatting; command unchanged.

@Strift Strift requested review from norkunas and curquiza September 12, 2025 09:25
@Strift Strift added this pull request to the merge queue Sep 12, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Sep 12, 2025
@Strift Strift merged commit 5cd4676 into main Sep 12, 2025
11 checks passed
@Strift Strift deleted the chore/update-config-v2 branch September 12, 2025 11:06
Strift added a commit that referenced this pull request Sep 18, 2025
* Update repository config for v2 (#787)

* Update CI workflows

* Update contributors

* Update empty string comparison

* lint

* Cast federation payload explicilty to an object (#757)

* Cast federation payload explicilty to an object

* Add unit test for multisearch federation array to object casting

---------

Co-authored-by: Thijs Kuilman <[email protected]>
Co-authored-by: Laurent Cazanove <[email protected]>

---------

Co-authored-by: Tomas Norkūnas <[email protected]>
Co-authored-by: Thijs Kuilman <[email protected]>
@coderabbitai coderabbitai bot mentioned this pull request Sep 18, 2025
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Anything related to maintenance (CI, tests, refactoring...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants