Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add filtering functionality to bids2table #6
Add filtering functionality to bids2table #6
Changes from 2 commits
1bf54b6
53d1de7
1044c27
8a1f81b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 don't think I would catch and re-raise a custom exception. I would just let the
KeyError
be raised. Just as informative I think and less code.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.
For a developer like you or myself you're right. However, my idea here is to raise an error that is informative to someone who has never opened the source code. Consider someone who calls
bids2table(..., filters={...})
If they get thrown aKeyError
at line 140 atdf = df[df["entities"][key].isin(value)]
then they'd have to go through the stack trace and figure out from there that thiskey
variable refers to one of their filters. That's a commitment beyond many end-users. If we're lucky they launch an issue for it, if we're unlucky they'll move to a different package. If they get thrown aInvalidFilterError
then they would barely have to glance at the error description to know that it's a user 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.
Fair point. The default
KeyError
message is very minimal and would probably be confusing. How about we re-raiseKeyError
with a more informative message? In general, I prefer not to use custom exceptions unless there's no good fit among the built-in exceptions or I need to handle the exception specially.