search

jina-ai / executors

internal-only
Apache License 2.0
31 stars 12 forks source link

fix(MongoDBStorage): check if embedding is present before .pop('embedding') #149

Closed sauravgarg540 closed 3 years ago

sauravgarg540 commented 3 years ago

Added a unit test case test_mongo_search_without_embedding to reproduce the issue.

cristianmtr commented 3 years ago

Added a unit test case test_mongo_search_without_embedding to reproduce the issue.

Hey @sauravgarg540

Thanks a lot for your contribution. But it doesn't seem your PR actually "fixes" anything, am I right? You are just adding a new test?

PS Our CI is not running the test because it relies on there being changes in the file in the Indexer's directory.

sauravgarg540 commented 3 years ago

Hi @cristianmtr, you are right this PR does not resolve the issue, in this PR I have only added a test case that shows the problem. I was not sure what you meant by

come up with a scenario in a test that shows the problem

so according to my understanding, I thought of providing you with a simple unit test case that reproduces the issue. If this test case matches your expectation, I will go ahead and create a new pull request that resolves the issue.

sauravgarg540 commented 3 years ago

Hi @cristianmtr, I have pushed the code that solves the issue so that review all the changes made by me. If there is something more to be done, please let me know.

cristianmtr commented 3 years ago

Hey @sauravgarg540

Seems like there is a linting problem with your code.

Can you check the Contributing guide and set up your pre-commit hook?

Also, can you add a requirements.txt to the tests folder with the required packages? (minimal would be pytest)

sauravgarg540 commented 3 years ago

hey @cristianmtr, I was using the wrong pre-commit-config.yaml which I have corrected now. You can review the code now.

For requirements.txt I have not installed any extra package for resolving the issue except the ones which are already mentioned in the documentation.

cristianmtr commented 3 years ago

Seems like there's still some linting issues present. Can you please check again?

sauravgarg540 commented 3 years ago

Hi @cristianmtr, there were some linting errors from the file which I have not touched, I manually used isort to make changes to the files that were throwing errors, I hope it works this time. Also, Flake8 was throwing errors in the following files:

jinahub/indexers/storage/MongoDBStorage/mongo_handler.py:36:89: E501 line too long (91 > 88 characters)
jinahub/indexers/storage/MongoDBStorage/mongo_storage.py:76:89: E501 line too long (92 > 88 characters)
jinahub/indexers/storage/MongoDBStorage/tests/conftest.py:40:89: E501 line too long (98 > 88 characters)

I have modified the above lines and the current pull request throws no lining errors on my system.