-
Notifications
You must be signed in to change notification settings - Fork 449
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
Removed extra conditions in _auto_identify_connection_string
method
#2038
Removed extra conditions in _auto_identify_connection_string
method
#2038
Conversation
|
||
elif item.startswith('mongodb://'): | ||
kwargs['data_backend'] = item | ||
kwargs['data_backend'] = item |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ChandanChainani thanks for the PR. Are you sure that these can be dropped like this? Do you get passing tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @blythed, Yes, except for llm and doc_string related test all the other tests are validated
Command
pytest ./test/unittest/ -svv --ignore=test/unittest/ext/test_llama_cpp.py --ignore=test/unittest/ext/llm --ignore=test/unittest/ext/transformers/test_llm.py --ignore=test/unittest/ext/transformers/test_llm_training.py --ignore=test/unittest/ext/transformers/test_transformers.py
Output
=============================================== short test summary info ===============================================
=============================================== short test summary info ===============================================
FAILED test/unittest/cli/test_cli.py::test_cli_info - subprocess.CalledProcessError: Command '('python', '-m', 'superduperdb', 'info')' returned non-zero exit status 1.
ERROR test/unittest/backends/ibis/test_query.py::test_renamings - NotADirectoryError: [WinError 267] The directory name is invalid: 'C:\\Users\\...\\AppData\\Local\\Temp\\tmpi9bnoy_0\\test.ddb'
=========================== 1 failed, 194 passed, 1112 skipped, 1 error in 85.44s (0:01:25) ===========================```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have update the code and there is one additional condition that I think can be added which is to make sure the user only supply supported db/file uri format please suggest (for supported db/file format we can maintain constants)
8b5e760
to
7c48427
Compare
7c48427
to
fcdb705
Compare
|
||
elif item.startswith('mongodb+srv://') and 'mongodb.net' in item: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @ChandanChainani , we need to check this pattern, because re_match(r'^[a-zA-Z0-9]+://', 'mongodb+srv://') is None == Ture
, then will raise an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jieguangzhou for this type of conditions we can improve by creating single condition which validates all the supported url types.
For example:
supported_url_types = ["mongodb+srv://", "mongodb://", "mongomock://"]
if not any(item.startswith(url_type) for url_type in supported_url_types):
raise Exception("url format not supported")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, could you update the pr with these changes?
|
||
if item.startswith('mongomock://'): | ||
kwargs['data_backend'] = item | ||
if re_match(r'^[a-zA-Z0-9]+://', item) is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use re.match
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jieguangzhou it is re.match
method I have created alias which is being used since we are only using single method from the re
package.
@ChandanChainani Thanks for your PR, Please update the CHANGELOG file, and update the review comments, we will merge the PR after the CI passes, thanks |
b8748a3
to
04010cf
Compare
No changes here for several weeks so closing. |
Description
Moved uri string validation case on the start and remove additional conditions as it seem unnecessary
Related Issues
ISSUE: NA
Checklist
make unit-testing
andmake integration-testing
successfully?