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

0.8.0 draft - Part 2 #3

Merged
merged 9 commits into from
Feb 24, 2018
Merged

0.8.0 draft - Part 2 #3

merged 9 commits into from
Feb 24, 2018

Conversation

jasonbosco
Copy link
Member

Renamed the 0.7.2-draft branch to 0.8.0-draft which caused Github to automatically close PR #1. So opening a new Part 2 PR and continuing the conversation here.

@kishorenc
Copy link
Member

@jasonbosco Noticed one problem:

 "/collections/{collectionName}" returns "#/definitions/Collection" schema

However, one of the fields that we should return in the collection summary end-point is num_documents which is actually not part of the collection schema. Currently, the collection summary end-point returns a document like:

{    
  "name": "companies",
  "num_documents": 1250,
  "fields": [
      {"name": "company_name", "type": "string"},
      {"name": "num_employees", "type": "int32"},
      {"name": "country", "type": "string", "facet": true}
  ],
  "token_ranking_field": "num_employees"
}

Likewise for GET /collections end-point.

@kishorenc
Copy link
Member

Moved the collection model into its own "collection" key like this:

{
    "num_documents": 1499
    "collection": {
        "fields": [
            {
                "facet": false,
                "name": "original_title",
                "type": "string"
            },
            {
                "facet": false,
                "name": "author_names",
                "type": "string[]"
            }
        ],
        "name": "goodreads_10k",
        "token_ranking_field": "ratings_count"
    },
}

Please update the corresponding API spec @jasonbosco - I have also re-published the docker image.

@kishorenc
Copy link
Member

kishorenc commented Jan 14, 2018

And, the latest API docs are here:

http://wreally.com/typesense-docs-0.8

I should wrap up the simple website in a couple of days. Just need the clients ready and we can do the beta. I really want to launch it before someone else launches a similar project as open source - that would be 2 years of work down the drain!

@jasonbosco
Copy link
Member Author

jasonbosco commented Jan 15, 2018

@kishorenc I had already accounted for this use case by setting num_documents as a read-only attribute of a collection:

screen shot 2018-01-14 at 11 13 34 pm

This way, we don't have to create a new response model for just returning collections.

When creating a new collection, since num_documents is a read-only field, you'd just not send that field in the POST payload.

@jasonbosco
Copy link
Member Author

I started working on the Ruby API client using Swagger Codegen. The code & method signatures that Swagger generated were unfortunately not too idiomatic Ruby-esque and leave a lot to be desired.

Swagger code-gen does allow you to customize the mustache templates that generate the code, but at that point you might as well write your own code rather than editing a template that generates your code. It also makes testing painful (testing code vs testing templates that generate code) and will be very hard to get people to contribute to the library in the future if there are multiple layers of abstraction.

So I'm now thinking I'll just write the Ruby client by hand myself.

That said, I'd say the Swagger API spec modelling exercise was still valuable to make the API consistent.

@kishorenc
Copy link
Member

kishorenc commented Jan 15, 2018 via email

@jasonbosco
Copy link
Member Author

@kishorenc I skimmed through a handful of companies from the list mentioned here and I only found one ruby library from DocuSign that had swagger-codegen style code but it was still modified to remove any references to Swagger in the code itself.

Some other companies I looked at: Fastly, Box, Bitly, ElastiSearch, HootSuite, PagerDuty, Wealthfront, Upwork. Their Ruby API's didn't look like Swagger Codegen generated.

@kishorenc
Copy link
Member

I found a generated client for Dwolla: https://github.com/Dwolla/dwolla-swagger-ruby

How does the above look? When you mean by "not too idiomatic" - can you give some example? Is that something that's tolerable given the overall effort saved?

@jasonbosco
Copy link
Member Author

Looks like they've deprecated that library and ended up writing their own library eventually:

screen shot 2018-01-15 at 7 39 26 pm

Also, like I suspected, even when they had the swagger-generated API client, they had to fork the swagger-codegen repo (circled above) and add their own custom mustache templates to customize the generated code to their needs. I don't think this additional layer of abstraction + complexity is worth it. Instead of editing the template to generate code we want and maintaining that, might as well write the code and maintain it.

re: Idiomatic Ruby, there are a couple of aspects:

Here's what swagger-codegen generates by default:

screen shot 2018-01-15 at 7 45 45 pm

First issue here is that it has the namespace prefix "SwaggerClient" attached to all classes which we have to edit out either once the files have been generated or in the swagger-codegen template, which is a pain. Once we do that, we can no longer rely on swagger-codegen in the future anyway - we'd have to do this every time we make updates to the API spec.

The next issue is that it creates a new class for each endpoint and then duplicates the object name in the method. Eg:
SwaggerClient::CollectionsApi.create_collection
SwaggerClient::CollectionsApi.delete_collection
SwaggerClient::CollectionsApi.get_collection
SwaggerClient::CollectionsApi.get_collections

I'd rather have the signatures as:

Typesense::Collections.create
Typesense::Collections.delete
Typesense::Collections.get
Typesense::Collections.get_all

The latter is simpler and more intuitive and what I'd expect from a Ruby library.

Finally, the generated code has no tests, only stubs. So we anyway have to write tests.

So the generated code is really only a starting point and requires way more maintenance and editing. Given our two-endpoint use-case, it will be easier and leaner to just start from scratch.

@kishorenc
Copy link
Member

Sigh! Looks like this itself would make a nice project! Okay, let's go with custom client. Hopefully the Ruby version can be used as a reference for other clients.

@jasonbosco
Copy link
Member Author

@kishorenc Could you confirm that you rolled this back: #3 (comment)

@kishorenc
Copy link
Member

Yes this was rolled back.

@kishorenc kishorenc merged commit bdf9cb0 into master Feb 24, 2018
@kishorenc kishorenc deleted the 0.8.0-draft branch February 24, 2018 03:53
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.

None yet

2 participants