Re: [PATCH 5/6] cli/new: support /<regex>/ in new.ignore

Subject: Re: [PATCH 5/6] cli/new: support /<regex>/ in new.ignore

Date: Sun, 01 Oct 2017 22:00:24 -0300

To: Jani Nikula, notmuch@notmuchmail.org

Cc:

From: David Bremner


Jani Nikula <jani@nikula.org> writes:
>>
>> One thing we eventually settled on in the query parser is that an
>> opening '/' without a trailing '/' is an errror. But perhaps it's fine
>> to take a more permissive approach here.
>
> I'm fine either way, I just chose to be permissive.
>
> So do I make the function void and drop the return values, or make it
> check and return errors?

I think I'd prefer to start strict, it's easier to become permissive later.

>
>>
>>> +
>>> +    if (! state->ignore_regex_length)
>>> +	return FALSE;
>>
>> It's a nitpick, even by the standards of this review, but I'd prefer an
>> explicit '> 0' check.
>
> ITYM (state->ignore_regex_length == 0) but ack.
>

yeah, thought of that after just after I sent it, but yes.
_______________________________________________
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

Thread: