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

Added to Query.search method & tests #33

Merged
merged 1 commit into from Oct 13, 2014
Merged

Added to Query.search method & tests #33

merged 1 commit into from Oct 13, 2014

Conversation

wearp
Copy link
Contributor

@wearp wearp commented Oct 11, 2014

to resolve #32

	added tests for Query.search method
@eugene-eeo
Copy link
Contributor

I think it's better to do getattr(re, self.re_method) or use different classes entirely, like QueryRegexSearch. Such conditionals should typically be avoided since they cause problems and repetition.

@msiemens
Copy link
Owner

since they cause problems and repetition.

I don't think using getattr is any better. Easiest solution I can see would be to pass the actual function as re_method, e.g:

return QueryRegex(self._key, regex, re_method=re.match)

@eugene-eeo
Copy link
Contributor

If we do that, then it is just a Query.test with a different constructor.

@msiemens
Copy link
Owner

Is there a problem with that?

@eugene-eeo
Copy link
Contributor

Not saying that it's a problem, just that the repr wouldn't update properly.

@msiemens
Copy link
Owner

Hmm... In any case match and search should produce different reprs as a query's hash value is calculated from it (needed when using queries as dict keys, see query caching). Otherwise this may happen:

>>> d = {
    where('key').matches('asd'): [...]
}
>>> where('key').search('asd') in d
True

@msiemens
Copy link
Owner

I'll merge this PR, but we should fix the repr problem before releasing the next version of TinyDB.

EDIT: Calculating the hash of a QueryRegex is broken even without this patch. Will work on it.

msiemens added a commit that referenced this pull request Oct 13, 2014
Added to Query.search method & tests
@msiemens msiemens merged commit f53f587 into msiemens:master Oct 13, 2014
@msiemens
Copy link
Owner

Mentined problems are fixed in 20d245c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Propose new Query method similar to re.search()
3 participants