[PYTHON-3036] Improve error message for unknown MongoClient options #1440
Conversation
caseyclements requested review from ShaneHarvey and removed request for a team 
test/test_client.py Outdated
| | ||
| def test_validate_suggestion(self): | ||
| """Validate kwargs in constructor.""" | ||
| self.assertRaises(ConfigurationError, MongoClient, auth="standard") |
There was a problem hiding this comment.
We can replace these two asserts with one that uses with self.assertRaisesRegex(...), for example:
mongo-python-driver/test/test_client.py
Lines 237 to 238 in 6c88c73
pymongo/common.py Outdated
| return option, value | ||
| except KeyError: | ||
| suggestions = get_close_matches(lower, VALIDATORS, cutoff=0.2) | ||
| raise_config_error(option, suggestions) |
There was a problem hiding this comment.
What do you think about refactoring this with the code block below? It seems like they are essentially identical besides the VALIDATORS vs URI_OPTIONS_VALIDATOR_MAP.
There was a problem hiding this comment.
I've added a note /hint on camelCase and suggested referring to docstring.
I've also opened a ticket to explore rationalizing the keys to kwargs: https://jira.mongodb.org/browse/PYTHON-4056
There was a problem hiding this comment.
Thanks I'll resolve the other comment about camelCase but this comment is still todo.
There was a problem hiding this comment.
Hi. If the original intent hasn't been resolved, would you please elaborate?
There was a problem hiding this comment.
Can you refactor this to remove the duplicated code so that there's only one call to try/except + get_close_matches/raise_config_error, eg:
def _get_validator(name, validators): lower = name.lower() try: return validators[lower] except KeyError: suggestions = get_close_matches(lower, validators, cutoff=0.2) raise_config_error(name, suggestions) … we only ever handle KeyErrors from the dict look up.
There was a problem hiding this comment.
@ShaneHarvey When you've got a moment, let's review my review :)
pymongo/common.py Outdated
| return option, value | ||
| except KeyError: | ||
| suggestions = get_close_matches(lower, VALIDATORS, cutoff=0.2) | ||
| raise_config_error(option, suggestions) |
There was a problem hiding this comment.
I've added a note /hint on camelCase and suggested referring to docstring.
I've also opened a ticket to explore rationalizing the keys to kwargs: https://jira.mongodb.org/browse/PYTHON-4056
test/test_client.py Outdated
| | ||
| def test_validate_suggestion(self): | ||
| """Validate kwargs in constructor.""" | ||
| self.assertRaises(ConfigurationError, MongoClient, auth="standard") |
pymongo/common.py Outdated
| return option, value | ||
| except KeyError: | ||
| suggestions = get_close_matches(lower, VALIDATORS, cutoff=0.2) | ||
| raise_config_error(option, suggestions) |
There was a problem hiding this comment.
Thanks I'll resolve the other comment about camelCase but this comment is still todo.
… in case provided. Refactor to use assertRaisesRegex
| for opt, value in options.items(): | ||
| normed_key = get_normed_key(opt) | ||
| try: | ||
| validator = URI_OPTIONS_VALIDATOR_MAP.get(normed_key, raise_config_error) |
There was a problem hiding this comment.
This code should still be using URI_OPTIONS_VALIDATOR_MAP but by calling validate() it now uses VALIDATORS. This is why I suggested the _get_validator function in my earlier comment.
pymongo/common.py Outdated
| | ||
| def _get_validator( | ||
| key: str, validators: dict[str, Callable[[Any, Any], Any]], normed_key: Optional[str] = None | ||
| ): |
There was a problem hiding this comment.
Typecheck is failing due to the missing return type here.
https://jira.mongodb.org/browse/PYTHON-3036
When an unknown kwarg was passed to MongoClient, the ConfigurationError wasn't helpful. I use the standard lib package
difflibto look for similar known options. It appears to work pretty well.First contribution to this mongodb. This PR / ticket doubles as SDLC tutorial. I've got a feature branch with a couple tests that I'll now use.
It will now look something like this: