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

Adopt as official library? #4

Closed
jasonbosco opened this issue Sep 19, 2020 · 19 comments
Closed

Adopt as official library? #4

jasonbosco opened this issue Sep 19, 2020 · 19 comments

Comments

@jasonbosco
Copy link
Contributor

Hi @AbdullahFaqeir,

This is Jason from Typesense. Thank you for your work on this PHP client for Typesense. Would you be interested in collaborating with us to make this an officially supported client library for Typesense?

We continue to add new features to Typesense Server and we’d like to work closely with you and collaborate to keep the PHP client up to date and in sync architecturally with our other client libraries.

We’ve been trying to reach you over email and haven’t heard back, so not sure if we have right email for you.

Let us know in any case!

@AbdullahFaqeir
Copy link
Contributor

Hello @jasonbosco ,

I've replied to your colleague Kishore but apparently he didn't get to see it I'm very sorry, yes I am very interested with working with you guys, you may do what you see proper regarding hosting the php client, and I'd be honored to support you guys with this client all the time.

@AbdullahFaqeir
Copy link
Contributor

Hi @jasonbosco

Any updates?

@jasonbosco
Copy link
Contributor Author

@AbdullahFaqeir I just looked at our help desk system and it looks your replies to our emails were being sent to spam for some reason :( Sorry about that.

Anyway, let's use this thread to continue the conversation.

Thank you for collaborating with us to make this client part of our official list. It looks good for the most part, except for some small changes needed for v0.15 of Typesense Server that we released last week:

  1. The import endpoint on the server now returns JSONL, with a content-type of 'text/plain' (instead of JSON like it used to be before). So we updated the createMany method in our other clients to explicitly parse this format by splitting on new lines, converting each line into an object/hash and then returning an array of hashes.

  2. We also added an import method to the clients so that users can access the raw JSONL response from the server, should they need to. So createMany accepts documents as input and returns documents as output, while import accepts JSONL as input and returns JSONL as output.

  3. We no longer split on \n in the export method. This way, users can pipe the response of this method directly to a file or to another index via the import method.

Here's a commit from the ruby library that summarizes all these changes for reference: typesense/typesense-ruby@df43976#diff-178eb2fa2f03af802382e0223bf1a666

Could you help make these changes?

In the meantime, as part of adopting your work as our official library, I'll email you a standard Contributor License Agreement (CLA) for you to review and sign. I'll also start adding PHP examples to our docs site.

We can then finally transfer the repo to our typesense github org. We'd of course continue to give you credit for your work in the README file (thank you again!) and have you as an admin for the repo.

Let me know if I can clarify anything.

@AbdullahFaqeir
Copy link
Contributor

Hello @jasonbosco,

I've adopted to the new updates from your ruby client, you can find it in commit and a new version has been released tagged with v2.0.4.

Please tell me if you need anything else to be updated and I'll be waiting for your CLA to sign.

Cheers

@jasonbosco
Copy link
Contributor Author

@AbdullahFaqeir Great! Two comments:

  1. Looks like create_many is snake cased, instead of being camel cased as createMany.
  2. import should take a plain string as input, unlike createMany. We just want to pass on the (JSONL) string that the user calls this method with to the server and let the server do the validation.

@AbdullahFaqeir
Copy link
Contributor

Hi @jasonbosco!

Though I checked the client and found it snake case 😅😅.

I'll fix both by tomorrow and will commit it, please update me with the progress of the documentation update with the php examples.

Cheers 🍻

@jasonbosco
Copy link
Contributor Author

jasonbosco commented Sep 23, 2020

@AbdullahFaqeir A couple of additional things I noticed:

  1. Now that we're adopting this as our official library, could we drop the Devloops namespace and use the main Typesense namespace? We'll continue to give credit to your company in the Readme.
  2. Looks like the package is currently published as devloopsnet/typesens-php (typo in typesens btw). As one of the final steps, we want to publish it under typesense/typesense-php.
  3. The generate_scoped_search_key method also needs to be update to camelCase.
  4. May I know what the purpose of offsetExists in Keys is? Can it not be done on the user's side, vs including it in the client library?
  5. Could you add a compatibility matrix similar to this: https://github.com/typesense/typesense-ruby#compatibility

Here's a PR where I've added PHP examples to the site: typesense/typesense-website#29

@aysark
Copy link

aysark commented Oct 4, 2020

I'm considering using typesense for an PHP/Laravel app so this client would be very nice when it becomes official.

@AbdullahFaqeir
Copy link
Contributor

Hey @jasonbosco!

I'm deeply sorry for the very late reply as I had a really hectic couple of weeks with work.

I've completed what you've asked in your last comment and find below description for each point you've asked for.

  1. create_many() is now createMany().

  2. import() now takes string as it's parameter which is the JSONL representation of the documents.

  3. I've dropped Devloops from the namespace and not it's Typesense only.

  4. As for the currently published package, it shall be taken offline once you complete the adopt of the package as it should be publish again from your side.

  5. The method is now rewritten in camelCase generateScopedSearchKey().

  6. This method is native in php ArrayAccess interface which gives a class to have the ability to be accessed as an array, and in this way I made this client follow the same usage as the other clients keys['key_id'], documents['document_id']...etc, so the offsetExists() is needed for the complete inner flow to run properly, you can read more about ArrayAccess here php ArrayAccess interface.

  7. The compatibility matrix has been added to the ReadMe file.

Again, I apologize for the later response.

I hope we can now proceed and finish the adoption of the repo to your side.

For the CLA, I've signed it once you've sent it to me.

Cheers.

@AbdullahFaqeir
Copy link
Contributor

Hello @aysark,

I'm really happy to be helpful with the client I wrote, and I'de like to inform you that I've already wrote a laravel-scout driver for this and it's simple and ready to use.

Cheers.

@jasonbosco
Copy link
Contributor Author

@AbdullahFaqeir No worries, I totally get it! Thank you again for your continued work on this library.

I did receive your signed CLA. So we're good on that front.

Could you go ahead and transfer this repo to the typesense github org? Github should continue to give you write access to the repo even after the transfer.

I'll then publish it under the typesense namespace.

@AbdullahFaqeir
Copy link
Contributor

@jasonbosco I'll transfer the repo now.

@AbdullahFaqeir
Copy link
Contributor

@jasonbosco I tried to transfer it to typesense but this happened You don’t have the permission to create public repositories on typesense

@jasonbosco
Copy link
Contributor Author

jasonbosco commented Oct 12, 2020

@AbdullahFaqeir Looks like Github only allows transfer of repos between orgs for users who are members of both the source and target orgs. So let's do this: I'm going to fork this repository into the Typesense org and I'll add you as a collaborator to it. Once I do that, I can send you a PR to update this repo's README to point people to the repository in the typesense github org.

I also realized that one additional benefit of this approach is that, the fact that you were the original author is explicitly called out in the Github UI - just below typesense/typesense-php, it should say forked from devloopsnet/typesense-php :)
See: #4 (comment)

@jasonbosco
Copy link
Contributor Author

@AbdullahFaqeir I've created the official fork here: https://github.com/typesense/typesense-php and added you as a collaborator (you should have received a notification from Github). I made some additional tweaks to the library, could you take a look in that repo and let me know if you notice anything off via the issue tracker there?

I've also sent you a PR (#6) to add a message to this repo's README, pointing people to the officially maintained version.

Once that PR is merged, I think we can call this issue done! Let me know if I missed anything.

@jasonbosco
Copy link
Contributor Author

jasonbosco commented Oct 13, 2020

@AbdullahFaqeir One quick update on this: after I created a fork and pushed some updates, Github showed this message in the UI: "This branch is 10 commits ahead of devloopsnet:master" - which made it look very odd, especially for an official library. So I've asked Github to convert this fork into a regular repo to prevent that message from showing up. This unfortunately also removes the "forked from X" message on top. But, I've left the Credits section in the Readme as is, giving you credit.

@AbdullahFaqeir
Copy link
Contributor

AbdullahFaqeir commented Oct 13, 2020

HELLO @jasonbosco,

I'm really proud of all this, thank you very much for the all the work you've putted to update it with further functionalities.

All is great right now, I'll update the read me of this repo to point to the new one.

You may close this issue with your last reply.

@jasonbosco
Copy link
Contributor Author

@AbdullahFaqeir Thank you once again for your work on this!

@jasonbosco
Copy link
Contributor Author

CC: @aysark fyi - the official library is now at: https://github.com/typesense/typesense-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

No branches or pull requests

3 participants