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

fix handling of regex cli options #70

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

taybin
Copy link
Contributor

@taybin taybin commented Nov 15, 2021

This improves the handling of whitelist-regex and blacklist-regex options which was simply confusing.

Before, you'd need to do something like --whitelist table1,table2 --whitelist-regex true, which also wasn't documented.

This improves it so it functions as desired, so that it can be either --whitelist table1,table2 or --whitelist-regex table1,table2. And so on for the blacklist feature as well.

Copy link
Member

@stlava stlava left a comment

Choose a reason for hiding this comment

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

I'm sorry for the huge delay in looking at this. The intention makes sense but I've made a note that the mutual exclusive check needs to be made before the if else chain.

}

regex = (len(wlr) != 0 || len(blr) != 0)
if filterConfigCount > 1 {
Copy link
Member

Choose a reason for hiding this comment

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

This check should go above the if else chain because filterConfigCount will never go above 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you mean. A better fix might be to change the if-else chain into just if-statements so that filterConfigCount actually adds up instead of defaulting to whatever the first match is in case of multiple, conflicting configurations.

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.

2 participants