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

Client refactor with new api names for JSON format #266

Closed
wants to merge 2 commits into from

Conversation

juchom
Copy link
Contributor

@juchom juchom commented May 18, 2022

Pull Request

What does this PR do?

This PR partially fixes #182 for JSON format

[BREAKING CHANGES]
AddDocumentsAsync is now AddDocumentsJsonAsync
AddDocumentsInBatchesAsync is now AddDocumentsJsonInBatchesAsync

UpdateDocumentsAsync is now UpdateDocumentsJsonAsync
UpdateDocumentsInBatchesAsync is now UpdateDocumentsJsonInBatchesAsync

The README and code samples have been updated to reflect the changes.

The test infrastructure has been updated to easily leverage it when we will add NDJSON and CSV.

I introduce a JsonHttpContent, this allow to remove some custom code from HttpExtensions (more will be removed later) and easily follow what Adding and Updating functions are doing.

Let me know if things are not clear, this PR is a bit big.

Copy link
Contributor

@alallema alallema left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @juchom,
Thank you for this PR it is always a pleasure to have such good contributions! And sorry for the delay, I just got this SDK.
I had a couple of comments or questions first:

  • the AddDocuments method is not destined to disappear only the specific functions (json, csv, ndJson) must be added

(more will be removed later)

  • Do you plan to add addDocument and other functions afterward?
    Thanks again

@juchom
Copy link
Contributor Author

juchom commented May 23, 2022

Hi @alallema,

No problem for the delay, just take your time and do not hesitate to ask me questions if something is not clear.

I thought that AddDocuments method would disappear in favor of Json, Ndjson and Csv ones.

What AddDocuments is supposed to do if we have support for the three formats ?

@juchom
Copy link
Contributor Author

juchom commented May 23, 2022

Ok, I went a bit fast, I've just checked the signature of the methods.

This is PR is not in line with the issue :)

I will make new ones.

@juchom juchom closed this May 23, 2022
@alallema
Copy link
Contributor

alallema commented May 23, 2022

What AddDocuments is supposed to do if we have support for the three formats?

The AddDocuments method is supposed to send an Object, Class or an Array according to the SDK, while the AddDocumentsJson, AddDocumentsCsv and AddDocumentsNdJson methods are supposed to retrieve this document in string format and send it as is.
To note: Before v0.23, all SDKs sent Content-Type: application/json on every request. After I added the possibility to send NdJson or Csv we left the sending of Content-Yype only for POST and PUT requests with the appropriate Content-Type. The AddDocuments method keep Content-Type: application/json header.
Hope it's more clear, sorry maybe the issue isn't clear enough.

@juchom
Copy link
Contributor Author

juchom commented May 23, 2022

This is fine, I didn't check the methods signatures :)

@juchom juchom deleted the juchom/json-rename branch May 24, 2022 08:18
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.

NDJSON/CSV methods to add and update documents
2 participants