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

Allow doc_id to be specified when using insert_multiple #389

Closed
wants to merge 1 commit into from

Conversation

waylonflinn
Copy link

Currently doc_id cannot be used with insert_multiple because no check is performed for the Document class. This PR adds the same code as used in insert to insert_multiple, thus allowing doc_id to specified for any item being added.

@msiemens
Copy link
Owner

msiemens commented Apr 9, 2021

Thanks for the pull request @waylonflinn! This looks good to me. Could you add a quick test case for this in tests/test_tables.py? Then I'd be ready to merge this

@ianhinder
Copy link

It looks like this would allow you to insert documents with document IDs that already exist in the table. insert currently checks that a supplied document ID does not already exist, as far as I can see. Probably this PR should be changed to check that the provided document ID is not already in the table with a similar assertion to that used in insert.

@benetherington
Copy link
Contributor

@waylonflinn If you want to pass on these requested changes, I can work on this.

@stale
Copy link

stale bot commented Jun 11, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Feel free to reopen this if needed. Thank you for your contributions ❤️

@ianhinder
Copy link

This issue hasn't gone away, and the PR is still useful. @waylonflinn - do you agree with my comment that an additional check should be added?

msiemens pushed a commit that referenced this pull request Feb 14, 2022
* removed assert, added functionality of  #389

replaced assert stement with if and implemented a way to add documents with their existing doc_id using insert_multiple (see  #389 for original PR).

* fixed bug
@msiemens
Copy link
Owner

This has now been implemented by #455 which has been merged 🙂

@msiemens msiemens closed this Feb 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants