Skip to content

Generalize String criteria matching#471

Open
earboxer wants to merge 4 commits intoemersion:masterfrom
earboxer:306-improve-criteria
Open

Generalize String criteria matching#471
earboxer wants to merge 4 commits intoemersion:masterfrom
earboxer:306-improve-criteria

Conversation

@earboxer
Copy link

Closes #306

Most string fields have been changed to support

  • Regex matching [app-name~=D..o]
  • Not equal to [summary!=""]
  • Bare specification [summary] (equivalent to [summary!=""])
  • Bare negation [!summary] (equivalent to [summary=""])

@earboxer
Copy link
Author

I included the operator OP_NONE, but it's not being used.

Originally I was thinking this could replace the 'spec' definitions, but didn't dig deep enough to see if this is the case or not...

Copy link
Owner

@emersion emersion left a comment

Choose a reason for hiding this comment

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

Thanks for the patch! Sorry I took way too much time to get back to this.

I think it's a bit confusing to introduce all of these new operators, but only support them on strings. For instance truthy/falsy are not supported on booleans.

Maybe operators could be introduced incrementally on all types they make sense, starting with e.g. regexp which only makes sense on strings.


enum operator { OP_NONE, OP_EQUALS, OP_REGEX_MATCHES, OP_NOT_EQUALS, OP_TRUTHY, OP_FALSEY };

struct mako_condition {
Copy link
Owner

Choose a reason for hiding this comment

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

This is specific to strings, maybe should be renamed to e.g. struct mako_string_criteria?

Copy link
Author

Choose a reason for hiding this comment

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

I think I was originally thinking that mako_condition could be used for all of the conditions (and maybe there are other operators that other types of conditions could use, like "in" or "less than"/"greater than" for urgency, group-index).

Since this PR is "Generalize String criteria matching", I didn't want to go too far out of scope and refactor all of the conditions.

But I'm not too invested in this, so if you really want the rename, I will probably do it.

Copy link
Owner

Choose a reason for hiding this comment

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

How would that work? The value field is a char *, but we'd need some other type for urgency/group-index/etc.

@earboxer earboxer force-pushed the 306-improve-criteria branch from 7a510ed to f2ea430 Compare March 11, 2025 00:24
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.

Generalize criteria regex matching

2 participants