[DX] Improve quality of Client class & related #514

stloyd · 2023-05-30T14:08:34Z

Pull Request

Related issue

Fixes n/a

What does this PR do?

The changes in this PR aim to improve the quality of the code:

moving property definition to traits that use them,
adding type-hints when possible,
adding array-shapes into the param definition when possible,
simplifying some code

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!

brunoocasali · 2023-05-30T14:40:59Z

Hi @stloyd thanks a lot for your contribution. Quick question: do these improvements introduce any breaking change?

Also, I have other PRs in my queue to review so I'll probably take a look at your code near the end of the week, ok?

Good to have you here!

stloyd · 2023-05-30T14:45:32Z

Hey, no problem about the later review.

About changes, those are mostly to prevent later bugs rather than introduce them, there is only one case when those changes could introduce BC break if someone would extend classes from this SDK and try to overwrite the methods.

From my perspective I would even suggest (or do) a PR that will lock all the classes with the final keyword, to prevent the BC breaks, and allow easier maintenance and a safer Developer Experience in the end.

Refs: https://ocramius.github.io/blog/when-to-declare-classes-final/

brunoocasali · 2023-05-30T14:55:25Z

As far as I know, the main integrations we have will not be impacted even after adding the final classes, but I will want to have the opinion of our stars.

📡 Signal to @mmachatschek (scout) and @norkunas (symfony).

stloyd · 2023-05-31T09:16:11Z

To not pollute this PR with discussion about final's I have added new task about it: #518

mmachatschek · 2023-06-01T08:30:02Z

Concerning the final topic. I hear a lot of negatives about this (in the Laravel community) to not use this feature, even though it looks like a good idea. Users that need to extend classes marked as final will need to wait for upstream or have to fork libs themselves if they run into issues.
Also there are workarounds to extend classes even if they are marked as final so there is no 100% guarantee that the class marked as final will be in the final state.

I haven't looked much into the changes on this PR but @brunoocasali takes care to test this thoroughly. I will try to test theses changes with a laravel scout project as soon as @brunoocasali has done his first review 👍

brunoocasali

I usually take the review time to improve my skills in the languages and learn with the experts, so if you don't mind I made some comments in that purpose, but also there is one about the ?array change that may not be time to do it, since in my view it is breaking!

Thanks a lot for your time, and I will wait your answer, thanks!! 🎉

src/Http/Client.php

src/Exceptions/InvalidArgumentException.php

src/Endpoints/TenantToken.php

brunoocasali · 2023-06-06T20:03:32Z

src/Endpoints/Delegates/HandlesSystem.php

@@ -32,14 +42,14 @@ public function stats(): array
        return $this->stats->show();
    }

-    public function generateTenantToken(string $apiKeyUid, $searchRules, ?array $options = []): string
+    public function generateTenantToken(string $apiKeyUid, $searchRules, array $options = []): string


This is a breaking change, no? Since we are now making a requirement to call with the array instead of an optional.

In a really rare case it can be a BC break, but it's optional already, as it has the default value $options = [] which will pass [] when the developer doesn't pass anything on its own.

But here it leads to another case described here: https://github.com/meilisearch/meilisearch-php/pull/514/files#r1221466672 which would mean that now the code is less fragile and more strict for things that are required to be set, or how they should be set.

src/Endpoints/Delegates/HandlesSystem.php

brunoocasali · 2023-06-06T20:08:57Z

src/Endpoints/Delegates/HandlesSystem.php

 trait HandlesSystem
 {
+    protected Health $health;


To understand it entirely, since the traits are a mechanism for code reuse, we are now defining the properties that are used inside of the trait internally instead of simply waiting for the class that will use Trait to define them, right?

If the statement above is right, why defining this in such a way is even possible? Do you know any history behind this language decision?

This change is to make DX better, while trait is for code sharing, they would fail badly when you don't declare required properties for them in the class that uses them (previous version of code), while with protected (which means shared within inheritance but still not publicly visible) means that developer can use methods by just adding those traits with not further changes.

In fact, those traits in this SDK are overkill IMHO, cause the code here is not really so abstract to be shared as much as it seems, cause those traits in fact are definitions of endpoints, which will not really change over time, yet they will not be shared outside of their scopes (you will not call 2-3 endpoints from 2-3 traits in same SDK method).

Perfect, thanks for the explanation!

norkunas · 2023-06-07T12:03:52Z

As far as I know, the main integrations we have will not be impacted even after adding the final classes, but I will want to have the opinion of our stars.

satellite Signal to @mmachatschek (scout) and @norkunas (symfony).

for me it looks like a good decision to make them final, as composition should be always preferred vs inheritance

brunoocasali

Thanks a lot for this contribution and all the discussions @stloyd!

bors merge

meili-bors · 2023-06-07T18:15:43Z

Build succeeded:

meili-bot · 2023-06-07T18:27:53Z

This message is sent automatically

Thank you very much for submitting your PR! Did you know that throughout the month of June we’re holding a rafle?
If you share the link to your merged PR in our #giveaway Discord channel, you’ll automatically join a lottery for a chance at winning some Meiliswag. 🙂
Don’t hesitate to join us: https://discord.com/channels/1006923006964154428/1111273670657200198

557: Update version for the next release (v1.3.0) r=brunoocasali a=meili-bot This version introduces features released on Meilisearch v1.3.0 🎉 Check out the changelog of [Meilisearch v1.3.0](https://github.com/meilisearch/meilisearch/releases/tag/v1.3.0) for more information on the changes. ⚠️ If you want to adopt new features of this release, **update the Meilisearch server** to the according version. ## 🚀 Enhancements * Add a base exception interface all exceptions must implement (#534) `@94noni` * ⚠️ **EXPERIMENTAL**: `setShowRankingScoreDetails` and `search(['showRankingScoreDetails' => boolean])` to show the scores of each relevant document returned in the search. * ⚠️ **EXPERIMENTAL**: `setVector` and `search(['vector' => [...]])` to enable vector search capabilities. * Add `setAttributesToSearchOn` and `search(['attributesToSearchOn' => [...]])` * `index->facetSearch(FacetSearchQuery params)` to search only in facets. ⚠️ The EXPERIMENTAL features are not covered by semver. To explicitly opt-in, check this [guide](https://www.meilisearch.com/docs/reference/api/experimental-features). ## 💅 Misc * Migrate docs hosting to Meilisearch subdomain (#553) `@Strift` * Update SDK docs link in README.md (#556) `@Strift` * [DX] Improve quality of Client class & related (#514) `@stloyd` * [DX] Add support for PHPUnit 10.1 (#516) `@stloyd` Thanks again to `@94noni,` `@Strift,` `@alallema,` `@brunoocasali,` `@norkunas` and `@stloyd!` 🎉 Co-authored-by: meili-bot <[email protected]>

mmachatschek · 2023-10-06T13:08:34Z

src/Http/Client.php

        $this->http = $httpClient ?? Psr18ClientDiscovery::find();
        $this->requestFactory = $reqFactory ?? Psr17FactoryDiscovery::findRequestFactory();
        $this->streamFactory = $streamFactory ?? Psr17FactoryDiscovery::findStreamFactory();
        $this->headers = array_filter([
            'User-Agent' => implode(';', array_merge($clientAgents, [Meilisearch::qualifiedVersion()])),
        ]);
-        if (null != $this->apiKey) {
-            $this->headers['Authorization'] = sprintf('Bearer %s', $this->apiKey);
+        if (null !== $apiKey) {


@brunoocasali unfortunately this made some troubles for environment keys that are passed in as empty string as mentioned here:

laravel/scout#769

Imho we should add phpdoc @param non-empty-string|null $apiKey

@norkunas I'm posting the issue description now for tracking

stloyd added 2 commits May 30, 2023 16:04

Improve quality of Client class & related

95654d3

Fixed TenantToken generator working with null values

89fbd2a

mmachatschek mentioned this pull request Jun 1, 2023

DX - Make SDK classes final when possible #518

Open

brunoocasali reviewed Jun 6, 2023

View reviewed changes

brunoocasali added enhancement New feature or request maintenance Anything related to maintenance (CI, tests, refactoring...) labels Jun 6, 2023

brunoocasali approved these changes Jun 7, 2023

View reviewed changes

meili-bors bot merged commit 1087cfa into meilisearch:main Jun 7, 2023

brunoocasali mentioned this pull request Jul 31, 2023

Update version for the next release (v1.3.0) #557

Merged

mmachatschek reviewed Oct 6, 2023

View reviewed changes

mmachatschek mentioned this pull request Oct 6, 2023

delete-all-indexes command not working with meilisearch/meilisearch-php 1.3.0+ laravel/scout#769

Closed

stloyd deleted the improve-quality branch October 6, 2023 13:18

mmachatschek mentioned this pull request Oct 6, 2023

Issue when upgrading from < 1.3 to >= 1.3 with api keys set to empty string #584

Closed

norkunas mentioned this pull request Oct 11, 2023

Fix passing empty masterkey #585

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DX] Improve quality of Client class & related #514

[DX] Improve quality of Client class & related #514

stloyd commented May 30, 2023 •

edited

Loading

brunoocasali commented May 30, 2023

stloyd commented May 30, 2023

brunoocasali commented May 30, 2023

stloyd commented May 31, 2023

mmachatschek commented Jun 1, 2023

brunoocasali left a comment

brunoocasali Jun 6, 2023

stloyd Jun 7, 2023

brunoocasali Jun 6, 2023

stloyd Jun 7, 2023

brunoocasali Jun 7, 2023

norkunas commented Jun 7, 2023

brunoocasali left a comment •

edited

Loading

meili-bors bot commented Jun 7, 2023

meili-bot commented Jun 7, 2023

mmachatschek Oct 6, 2023

norkunas Oct 6, 2023

mmachatschek Oct 6, 2023

[DX] Improve quality of Client class & related #514

[DX] Improve quality of Client class & related #514

Conversation

stloyd commented May 30, 2023 • edited Loading

Pull Request

Related issue

What does this PR do?

PR checklist

brunoocasali commented May 30, 2023

stloyd commented May 30, 2023

brunoocasali commented May 30, 2023

stloyd commented May 31, 2023

mmachatschek commented Jun 1, 2023

brunoocasali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

norkunas commented Jun 7, 2023

brunoocasali left a comment • edited Loading

Choose a reason for hiding this comment

meili-bors bot commented Jun 7, 2023

meili-bot commented Jun 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stloyd commented May 30, 2023 •

edited

Loading

brunoocasali left a comment •

edited

Loading