Add ParseCallbacks::allow_item() and ParseCallbacks::block_item().#3245
Add ParseCallbacks::allow_item() and ParseCallbacks::block_item().#3245nicopauss wants to merge 1 commit intorust-lang:mainfrom
Conversation
Like `allowlist_item` and `blocklist_item` options, add new methods to `ParseCallbacks`, `ParseCallbacks::allow_item()` and `ParseCallbacks::block_item()`, to be able to allow and block items from being generated. `allowlist_*` and `blocklist_*` options work with regexes and are inserted in a RegexSet. There are two issues with this approach: 1. In some cases we want to have more flexibility than just using regexes. If we want to have dynamic conditions to allow or block items, using regexes can be limited. 2. RegexSet scales linearly with the number of elements inserted. This means that if we have a huge number of items that we want to allow or block, regexes and RegexSet are not necessarily the most appropriate data structures. Using new methods in `ParseCallbacks` solves these two issues. We can manually decide the appropriate rules and data structure to match the items. `ParseCallbacks::allow_item()` and `ParseCallbacks::block_item()` are always called after the `allowlist_*` and `blocklist_*` options, and allow or do not block the items by default respectively.
nicopauss
force-pushed
the
intersec/add-parse-callbacks-methods
branch
from
430055d to
89385ad
Compare
| impl ParseCallbacks for AllowItem { | ||
| fn allow_item(&self, item: ItemInfo) -> bool { | ||
| item.name.starts_with("allowed_") | ||
| } | ||
| } | ||
|
|
||
| #[derive(Debug)] | ||
| struct BlockItem; | ||
|
|
||
| impl ParseCallbacks for BlockItem { | ||
| fn block_item(&self, item: ItemInfo) -> bool { | ||
| item.name.starts_with("blocked_") | ||
| } | ||
| } |
There was a problem hiding this comment.
What's the behavior if both of these return true for an item? Will it be allowed or blocked? Seems like bad design to allow such conflicting option in the API (I realize this is already allowed by the current allowlist / blocklist regexes, such is life ...)
Could this API look something like:
enum AllowOrBlockItem {
AllowItem,
BlockItem
}
and then in parse callbacks, have:
fn allow_or_block_item(...) -> Option(AllowOrBlockItem)
Where a None return means "no override" and a Some means allow or block, per the returned value?
Bonus points if you can only call the callback once instead of twice.
I'm very new to bindgen codebase, just some ideas, take with a grain of salt etc.
There was a problem hiding this comment.
Sorry for the late reply.
The issue with your approach is that the callback will effectively be called twice in case the item is not blocked, first to check if the item is blocked, and then when checking if the item is allowed.
I don't think this is a desirable behaviour.
And I don't think reworking the whole system just to be able to call this callback once is appropriate.
This also introduces a different API than the Builder methods approach (allowlist_item and blocklist_item), which can be confusing.
What I can do, however, is improve the documentation and how it interacts with allowlist_item and blocklist_item.
2 participants
Like
allowlist_itemandblocklist_itemoptions, add new methods toParseCallbacks,ParseCallbacks::allow_item()andParseCallbacks::block_item(), to be able to allow and block items from being generated.allowlist_*andblocklist_*options work with regexes and are inserted in a RegexSet.There are two issues with this approach:
Using new methods in
ParseCallbackssolves these two issues. We can manually decide the appropriate rules and data structure to match the items.ParseCallbacks::allow_item()andParseCallbacks::block_item()are always called after theallowlist_*andblocklist_*options, and allow or do not block the items by default respectively.