Configure Csv delimiter

[alallema]

Add the ability to provide a CSV delimiter for adding and updating CSV documents as in the specification

SDK requirements: meilisearch/integration-guides#251

[brunoocasali]

I left some comments, if you apply the test ones please apply to both of the test cases :)

[brunoocasali]

I think we could reduce this csv file to a few lines :)

[alallema]

Yes! But thank @sanders41, the tests have been missing from the package build since #367. In case you wonder

[brunoocasali]

I don't like that much to see these if conditions because they usually hide a code that will grow indefinitely, WDYT about this?

Suggested change

	parameters = {}
	if primary_key:
	parameters["primaryKey"] = primary_key
	if csv_delimiter:
	parameters["csvDelimiter"] = csv_delimiter
	if primary_key is None and csv_delimiter is None:
	parameters = { "csvDelimiter": csv_delimiter, "primaryKey": primary_key }
	parameters = dict((k, v) for k, v in parameters.items() if v)
	if primary_key is None and csv_delimiter is None:

[sanders41]

This will work, but I would argue it is less readable and not obvious what you are doing. If you do go this direction you could clean it up some:

parameters = {k: v for k, v in parameters.items() if v}

With either direction it could also be worth it to short circuit so parameters don't get created and run if not needed:

    def _build_url(
        self,
        primary_key: Optional[str] = None,
        csv_delimiter: Optional[str] = None,
    ) -> str:
        if primary_key is None and csv_delimiter is None:
            return f"{self.config.paths.index}/{self.uid}/{self.config.paths.document}"

           parameters = { "csvDelimiter": csv_delimiter, "primaryKey": primary_key }
           parameters = {k: v for k, v in parameters.items() if v}

        return f"{self.config.paths.index}/{self.uid}/{self.config.paths.document}?{parse.urlencode(parameters)}"

[alallema]

I agree with sanders41. In general, I prefer to avoid using the magic one-liners of Python to avoid complex understanding.
And we should not have to add a new one. But if we do, I will change it for your code!

[brunoocasali]

Is this really needed?

[alallema]

Not really, but all the other tests are like that, so I find it weird to remove it for this one.

[brunoocasali]

Also the task status, if the wait did not work your test will fail anyway right?

[sanders41]

It's possible for the task to complete, but the status to be failed right? I think this is what she is testing, that it was successful?

[brunoocasali]

I got the idea, and my point is the assertion has no practical value because if the task fails, the assert task.details["receivedDocuments"] == 53 will fail and also the following assertions after get_documents.

We have 6 assertions in this test case, but only the last two are checking what the use case wants to assert (+ the receivedDocuments assertion), which is to verify if the documents were indexed properly.

I know test code is supposed to be explicit instead of implicit, but in this case, I struggle to find good reasons to keep those assertions everywhere.

[brunoocasali]

When you read this:

def test_add_documents_csv_with_delimiter(empty_index, songs_csv_custom_separator):
    """Tests adding new documents to a clean index."""

    index = empty_index("csv-delimiter")
    response = index.add_documents_csv(songs_csv_custom_separator, csv_delimiter=";")
    task = index.wait_for_task(response.task_uid)
    assert task.details["receivedDocuments"] == 53

    documents = index.get_documents().results
    assert documents[1].country == "Europe"
    assert documents[4].artist == "Elton John"

Could you let me know if you missed the removed assertions?