Add methods to add JSON, CSV and NDJSON document #257
Conversation
...or not before the request. By exposing the request options as a Hash param of http_request. Reason: - Currently, we transform the HTTP request body into JSON before each request. - Now, we may have body of different String types (JSON, NDJSON, CSV) that shouldn't be transformed into JSON. - Thus, we're exposing a flag to tell when a body should be transformed or not into JSON. - I've decided to use a flag over a complex rules that could check the content along with headers because ideally, the body transformation should be responsibility of a level above that knows about the content. Other benefit: - Improve options' organization and its default values, as they were spread around. - In the future: this could be even extracted into another class, maybe as Value Object - Note: RuboCop is complaining about "too many optional params" in http_post
- Fix the aux method that merge request options. - Skip to_json transformation for these methods. - Add related tests - Doesn't add in batches. - Doesn't support key transformations from camelCase to snake_case
...after too many optional params offense in MeiliSearch::HTTPRequest#http_post
|
Hello @thicolares |
|
bors try |
tryBuild succeeded: |
There was a problem hiding this comment.
Thank you so much @thicolares
And so sorry for the delay, I was really busy
| { "myId": 123, "title": "Pride and Prejudice", "comment": "A great book" }, | ||
| { "myId": 456, "title": "Le Petit Prince", "comment": "A french book" }, | ||
| { "myId": 1, "title": "Alice In Wonderland", "comment": "A weird book" }, | ||
| { "myId": 1344, "title": "The Hobbit", "comment": "An awesome book" }, | ||
| { "myId": 4, "title": "Harry Potter and the Half-Blood Prince", "comment": "The best book" } |
There was a problem hiding this comment.
If you really want to make this test relevant, you might want to use a primary key that does not contain id since MeiliSearch automatically infers it in this case. For example "refNumber"
There was a problem hiding this comment.
@curquiza mm.. got it. I'll do that.
|
Hello @thicolares sorry there are now git conlficts with |
|
@curquiza sure! Sorry about the delay. I believe I'll make the change and push it today :) |
|
Take your time, I'm on holiday for two weeks in a few hours ☀️ So I will review when I'll come back |
# Conflicts: # .rubocop_todo.yml # lib/meilisearch/http_request.rb # spec/meilisearch/index/documents_spec.rb
- The transform_body method was extract from http_request during c8144ac - Now, the only possible conversion is to_json - Then, I rename it to make it not related anyway to the extracted method transform_body
thicolares
force-pushed
the
add-add-document-methods-by-its-type
branch
from
5b95c96
to
9d94183
Compare
- To add a bit more significance to the tests - Because meilisearch automatically infers the primary key if it contains "id" in it -- https://docs.meilisearch.com/learn/core_concepts/documents.html#meilisearch-guesses-your-primary-key
|
@curquiza it's done! |
There was a problem hiding this comment.
Thanks @thicolares, for the PR and your patience!
bors merge
|
Merge conflict. |
|
bors merge |
Addresses the following items of #243:
Changes
Tip: in order to ease the code review, you can start reviewing it commit by commit.
http_post. Motivations:transform_body?in order to decide ifhttp_requestshould or not transform the body to JSON;transform_body?,headersand other options' default values that were spread around into a single Hash;