ENH: Sieve variable support

[monoidic]

This fixes issue #2486

[sebix]

Wow, very neat again!

@kamil-certat as you have a use-case of this feature, do you want to have a look at the diff?

[kamil-certat]

I love the change, as well as the refactoring and formatting against black/ruff. I have some NIT comments and one or two clarification questions, but also a one big one: what has happened to the test test_extra_dict? Have you found it duplicated (if so, please remove test files and say what duplicates it)?

Please also update the bot documentation to describe variables as well as the changelog.

And as a discussion, I have two things I'm unsure, but I don't find it blocking:

1. a variable can be in place of typed values, like Bool. Shouldn't we then ensure in code that the variable has the correct value?
2. variables look like extremely promising for the future, but I also ask myself - wouldn't it be also nice just allow syntax like ifd.field1 = idf.field2? The current use case looks like only supporting this.
3. I'd think about extending the validation - I haven't seen if there is any place where we would try to statically validate that the requested variable was previously defined

[monoidic]

what has happened to the test test_extra_dict?

Accidentally discarded newer commits to the Sieve bot. I noticed it in the commit diff ~a week ago, but got distracted and forgot to address it...

a variable can be in place of typed values, like Bool. Shouldn't we then ensure in code that the variable has the correct value?

This has actually been addressed in the code. The resolve_value method which resolves variables (if needed) has a parameter, expected_types, expecting a type or tuple thereof. If specified, it raises an exception if the variable's type is not as expected.
Or do you mean that an expected type could somehow be specified in the Sieve file itself as well?

variables look like extremely promising for the future, but I also ask myself - wouldn't it be also nice just allow syntax like ifd.field1 = idf.field2? The current use case looks like only supporting this.

At the moment, yes.
The parsing end would technically accept both append extra.list $elem as well as append $list $elem at the moment (with the former appending a value given in a variable to a list in the event, and the latter working purely with variables), though the processing end does not handle variables as keys yet.
Allowing event keys to be used like that could be an option, but seems like a slight downgrade feature-wise (e.g any temporary variables which are not meant to end up in the event itself would have to exist under random keys of the event and be cleared out afterwards) while requiring all the same new validation that's been added for variables anyway.
Though for some cases, it would be more ergonomical.

I'd think about extending the validation - I haven't seen if there is any place where we would try to statically validate that the requested variable was previously defined

I'd expect check() to be the place for this.

[kamil-certat]

Thanks for the explanation! It sounds sufficient for me.

Before it gets merged, I'd ask you to verify / add a test that check() would reject a sieve file with variable used without being declared beforehand

[sebix]

Merging this, based on @kamil-certat's positive review.

For the last request on a check for undeclared variables, I opened #2526