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

Query with invalid paramter should raise an error #67

Merged
merged 8 commits into from Apr 12, 2019

Conversation

andbag
Copy link
Member

@andbag andbag commented Apr 8, 2019

Parameters can be used for a query that are not supported by the indexes. The user does not receive a feedback about the invalid used parameters. Results of a query could therefore misinterpreted. This behavior has already been identified

# Make sure that range tests with incompatible paramters
# don't return empty sets.
but the strategy is simply to ignore the invalid parameter. I therefore suggest that instead of skipping invalid parameters, an error should be raised.

@andbag andbag requested a review from hannosch April 8, 2019 15:22
@icemac icemac requested a review from d-maurer April 9, 2019 06:10
Copy link
Member

@icemac icemac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a good idea. As it changes the behaviour I'd suggest to bump the version to the next major one (in CHANGES.rst and setup.py) and add a description of the issue and the solution to the change log.

Copy link
Contributor

@d-maurer d-maurer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the error comes from the user, I would prefer ValueError over RuntimeError.

Putting the test into a new options property causes code duplication: the access logic is now both in __init__ as well as options. I suggest to put the test into the set method - and make options a passive attribute (rather than an active property).

Line 109 could be slightly simpified: = value instead of = value.lower() (value has been lowered a few lines before).

Copy link
Contributor

@d-maurer d-maurer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am happy.

@andbag andbag removed the request for review from hannosch April 9, 2019 12:59
@andbag
Copy link
Member Author

andbag commented Apr 9, 2019

@icemac I have created an entry in the change log. I don't have any practice in code management for the community. Therefore I ask you to do the next steps as soon as the right time is reached.

@icemac icemac changed the title Query with invalide paramter should raise an error Query with invalid paramter should raise an error Apr 12, 2019
@icemac icemac merged commit 64b1f3b into master Apr 12, 2019
@icemac icemac deleted the fix_check_query_options branch April 12, 2019 06:25
@icemac
Copy link
Member

icemac commented Apr 12, 2019

@andbag Thank you for fixing this issue. Should I cut a release now or are you planning other fixes which should go into the next release?

@andbag
Copy link
Member Author

andbag commented Apr 12, 2019

@icemac Thank you for the merge, it would be nice if the PR #68 would also be included in the next release.

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

Successfully merging this pull request may close these issues.

None yet

3 participants