Subject (and possibly other rules) with quotes

[legeana]

Quotes inside subject produce unexpected result, they basically remove quoting. Escaping is not really supported in GMail so it would be nice to print an error message if this happens to not confuse users.

    {   
      filter: {
        subject: '"hello world"',
      },  
      actions: {
        labels: ["testing"],
      },  
    },  

--- Current
+++ TO BE APPLIED
@@ -1 +1,5 @@
+* Criteria:
+    subject: ""hello world""
+  Actions:
+    apply label: testing
 

[mbrt]

You don't really need to put quotes in your filters, they are automatically added.

    {   
      filter: {
        subject: "hello world",
      },  
      actions: {
        labels: ["testing"],
      },  
    },

Results in:

--- Current
+++ TO BE APPLIED
@@ -1 +1,5 @@
+* Criteria:
+    subject: "hello world"
+  Actions:
+    apply label: testing

Additional possibilities:

• If at any time the automatic quoting bothers you, you can use the query operator. That will leave your stuff as is.
• If you need any logic like "hello" and "world", "hello" or "world", you can use the native operators and, or, along with subject.

I hope it helps.

[legeana]

You don't really need to put quotes in your filters, they are automatically added.

Yea I understand that now. My point is not that it should be supported. What I am saying is if there is a quote inside current behavior is confusing. Since it doesn't make sense to put a quote inside it's better to prohibit it/print a warning.

automatic quoting bothers you

It doesn't :) I actually quite liked it. However gmailctl should check if there are quotes inside and warn users that this will produce syntactically invalid result instead of silently doing so, or better not do it at all.

[mbrt]

You're right, I can print a warning if that happens. Contributions also welcome!

[mefuller]

@mbrt if this and #77 are still of interest, I would be happy to work on them as part of better familiarizing myself with golang

[mbrt]

Sure! I think the best place to put the fix would be here:

gmailctl/internal/engine/filter/convert.go

Lines 175 to 191 in 582979d

	func generateLeafAsString(leaf *parser.Leaf) (string, error) {
	needEscape := leaf.Function != parser.FunctionQuery && !leaf.IsRaw
	query := joinStrings(needEscape, leaf.Args...)
	if len(leaf.Args) > 1 {
	var err error
	if query, err = groupWithOperation(query, leaf.Grouping); err != nil {
	return "", err
	}
	}
	switch leaf.Function {
	case parser.FunctionHas, parser.FunctionQuery:
	return query, nil
	default:
	return fmt.Sprintf("%v:%s", leaf.Function, query), nil
	}
	}

We can just return an error if a leaf is already quoted and not marked as IsRaw.

[mbrt]

Or perhaps even better, to make this function fail if the thing to quote is already quoted:

gmailctl/internal/engine/filter/convert.go

Lines 236 to 238 in 582979d

	func joinEscaped(a ...string) string {
	return strings.Join(escapeStrings(a...), " ")
	}

[GMNGeoffrey]

I just ran into this behavior change when trying to update my filters. Are you sure it's correct? I had a lot of '" in my filters, specifically in the "to" line, which I believe were required. Specifically, in the case that you want to filter for emails sent to "foo+bar", if you need to write filter: {to: '"foo+bar"'},. If you just use filter: {to: "foo+bar"}, then it matches emails sent to both of "[email protected]" and "[email protected]". I propose to revert. Sorry I didn't report earlier. I only noticed when attempting to edit my configuration today.

[mbrt]

Ow, sorry about that. Perhaps though foo+bar should be auto-quoted?

I'm fine with reverting for now. TBH I wasn't expecting anyone to use quotes inside to:.

Could you also explain why foo+bar matches both [email protected] and [email protected]? Is it because + is interpreted the same as to:(foo bar)?

[GMNGeoffrey]

I think that's why, but I'm honestly not sure: it's just something I observed when setting up my filters. Auto-quoting + also seems reasonable to me. I think it would just be a matter of adding it to

gmailctl/internal/engine/filter/convert.go

Line 258 in 6f10365

if strings.ContainsAny(a, " \t{}()") {

. I'm (almost) always a proponent of revert first though. Reverting shouldn't remove any functionality that someone could be relying on, I don't think. It will just remove the extra guardrail for a bit.

As a meta-note: I wonder if there would be a better way to roll out new validations such that issues like this could be flagged earlier. The problem is that the validations only run when someone tries to apply their config, which usually only happens when they're editing. It would be nice if the config were reparsed as part of updating gmailctl. I don't think go install has any hooks for custom logic though. At the least, it might be nice if gmailctl edit tried to parse the existing config before opening the editor. That way someone doesn't make a bunch of edits only to discover that their config is invalid in some other place.

[mbrt]

Reverting sounds good for now. I'll take a look at a proper fix later on.