Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP - support latest version, add typehint, add static checks, and add tests #53

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

bepsvpt
Copy link

@bepsvpt bepsvpt commented Dec 10, 2023

Pull Request Not Yet Completed

Feel free to provide any ideas and suggestions.


This Pull Request completely reworks this package to achieve the following goals:

  1. Remove non-API-related features, such as health check, nodes, etc.
  2. Drop non-active-support PHP versions(< 8.2).
  3. Support the latest Typesense version(0.25.1).
  4. Add strict types to all parameters, methods, and classes.
  5. Add static checks(PHPStan).
  6. Add tests(Pest) and use GitHub Actions to run tests automatically.
  7. Change coding style linter to Pint.

Implemented APIs:

  • Collections (2023-12-10)
  • Documents (2023-12-10)
  • Search
  • GeoSearch
  • Vector Search
  • Federated / Multi Search
  • Analytics & Query Suggestions (2024-03-24)
  • API Keys (2024-03-24)
  • Curation (2024-03-24)
  • Collection Alias (2024-03-24)
  • Synonyms (2024-03-24)
  • Cluster Operations (2024-03-24)

Documentation:

  • README
  • CHANGELOG
  • UPGRADE

To be supplemented...

@bepsvpt bepsvpt changed the title WIP, 5.0 - rewrite the whole sdk WIP - add typehint, static checks, and tests Dec 10, 2023
@bepsvpt bepsvpt changed the title WIP - add typehint, static checks, and tests WIP - support latest version, add typehint, static checks, and tests Dec 10, 2023
@bepsvpt bepsvpt changed the title WIP - support latest version, add typehint, static checks, and tests WIP - support latest version, add typehint, add static checks, and add tests Dec 10, 2023
@jasonbosco
Copy link
Member

@bepsvpt I appreciate the PR, but we are unfortunately unable to entertain a full rewrite of the library, because we want to maintain consistency in the code layout between all the language libraries we officially maintain.

@thePanz
Copy link

thePanz commented Mar 22, 2024

@bepsvpt I appreciate the PR, but we are unfortunately unable to entertain a full rewrite of the library, because we want to maintain consistency in the code layout between all the language libraries we officially maintain.

what do you mean with "consistency in the code layout between all the language libraries"? different languages use a different standard in the project structure, which helps developers keeping their processes lean when dealing with multiple libraries.

I do not see a blocker introducing static-code analysis (Phpstan or PSalm) and strict-types, and tests is a blocking issue.
I was personally testing out if TypeSense would be a good fit for our project, but from the missing strict-types, no static analysis and no tests of this library I initially thought that this PHP implementation was abandoned.

@jasonbosco
Copy link
Member

jasonbosco commented Mar 22, 2024

I'd be down to accept a PR to add types, and any static analysis tools, to the existing library.

What I meant was, I'd not be down to change the internal file structure and method parameter signatures, to reduce maintenance overhead as we add new features.

@thePanz
Copy link

thePanz commented Mar 22, 2024

I see your point, but I still believe that the current implementation is still leading to headaches.

An example is: how do you know if a collection exists? (this is just one of the examples I found out):

Try 1:

$this->client->getCollections()['collection_name];

Does not work, as the collection is always returned/generated even if it does not exist on the server.

Try 2:

$this->client->getCollections()->offsetExists('collection_name');

this is always false if the API is not yet called under the hood

Try3:

$this->client->getCollections()->retrieve();
// Then one of the (1) or (2) abve

Nope, the API call is invoked, but the results not used to populate the collection

Try4 - conter intiutive (IMO)

$this->client->getCollections()['collection_name]->retrieve()`

Additional notes:

I believe there is a very low DX with this client, which should be tackled IMO
I was close to drop Typesense integration, just because of such bad DX I had

how can we help to make the PHP client better?

@jasonbosco
Copy link
Member

The actual method call to retrieve a collection is:

$client->collections['companies']->retrieve();

Essentially collections is an "array" of collection objects that you call retrieve() on to make an API call to the server.

Documented under the PHP tab in the docs: https://typesense.org/docs/0.25.2/api/collections.html#retrieve-a-collection

It's been a long time since I wrote production-level PHP, but is that not idiomatic PHP?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants