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

File analyzer API doesn't have a good way to pass arguments to instantiated analyzers #185

Open
rsmmr opened this Issue Oct 5, 2018 · 5 comments

Comments

Projects
None yet
3 participants
@rsmmr
Copy link
Member

rsmmr commented Oct 5, 2018

When adding a file analyzer with add_analyzer, there's no good way to pass arguments to the new analyzer instance (say, if it were an analyzer to extract certain tags from a data stream, I would to want tell it which tags to extract). There's a record AnalyzerArgs but that's kind of an odd thing: right now it has two callbacks for the DATA_EVENT analyzer; but it's passed in for analyzers of any kind. One could extend that type with further arguments for other analyzers, but having one type for all analyzers doesn't seem like the right mechanisms.

@sethhall

This comment has been minimized.

Copy link
Member

sethhall commented Oct 5, 2018

I've been wondering if Files::add_analyzer should return some sort of handle? I sort of get the sense that it should. It would make some stuff significantly easier.

@sethhall

This comment has been minimized.

Copy link
Member

sethhall commented Oct 5, 2018

Here's some imaginary code....

local x: AnalyzerHandle = Files::add_analyzer(f, FILES_EXTRACT);
Files::add_argument(x, "cutoff", "1024");

And if x was defined as a global, at some point you could do this...

Files::remove_analyzer(x);

I don't know how great this proposal is, but it would make some file handling activities much more convenient.

@jsiwek

This comment has been minimized.

Copy link
Member

jsiwek commented Oct 5, 2018

Having a separate Files::add_argument could be weird in case where an analyzer needs the argument as part of the initialization done during Files::add_analyzer ?

But yes, an alternative would likely be using a variadic functions to supply arguments (currently script-layer functions cannot be variadic, but BIFs can).

However, then you are doing type-checking manually in the core for each analyzer instead of relying on the scripting language to type-check things for you. It's also a bit more manual labor to document such an API and you are relying on the comments to be always reliable in telling you want analyzer tags take what arguments.

@sethhall

This comment has been minimized.

Copy link
Member

sethhall commented Oct 5, 2018

Yeah, breaking out argument adding is a dumb suggestion on second thought. The variadic suggestion is cool, but could be a bit weird from a scripting perspective. At least with the redef-ed argument list thing we have now we can document the arguments that can be passed to an analyzer.

The other part of this is being able to remove an analyzer by it's handle. Jon, remember how we couldn't figure out a good way to specify which analyzer to remove for the case where you might have multiple data analyzers attached to a file (as an example). At the moment we're using the set of arguments given to an analyzer as the way to identify the particular analyzer instance.

@jsiwek

This comment has been minimized.

Copy link
Member

jsiwek commented Oct 5, 2018

The other part of this is being able to remove an analyzer by it's handle. Jon, remember how we couldn't figure out a good way to specify which analyzer to remove for the case where you might have multiple data analyzers attached to a file (as an example). At the moment we're using the set of arguments given to an analyzer as the way to identify the particular analyzer instance.

Yeah, good memory -- looks like file analyzers are identified via the composite of the tag + AnalyzerArgs record. So I guess that could just mean either we need a new way of identifying a file analyzer that can be used in both core and script land or else a variadic version of Files::add_analyzer would be returning essentially the existing composite (tag, args) tuple that you later pass into Files:remove_analyzer.

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