[prev in list] [next in list] [prev in thread] [next in thread] 

List:       squid-dev
Subject:    Re: Reading ACL configuration files every request
From:       Alex Rousskov <rousskov () measurement-factory ! com>
Date:       2011-11-09 15:52:40
Message-ID: 4EBAA1C8.4070408 () measurement-factory ! com
[Download RAW message or body]

On 11/09/2011 08:18 AM, Henrik Nordström wrote:

>> Regarding making FORMAT optional to external_acl_type, this is a trivial
>> change, just didn't think there was any valid use of it when the
>> directive was implemented. Please try the attached trunk patch which
>> also adds %%{statictext} just in case someone needs to fake format
>> arguments to some helper.


>  	  %%		The percent sign. Useful for helpers which need
> -			an unchanging input format.
> +			an unchanging but not blank input format.

The "Useful for ..." phrase makes no sense in this context for
uninitiated (before and especially after the above quoted change).
Unchanging input format? Blank input format? It may be meaningful in a
commit message, when describing why the %-escape sequence was added but
here it only confuses the reader. I would remove that phrase completely.


> +	  %%{..}	Staic text

It feels wrong to use %% both for specifying raw percent sign and for
the string quoting mechanism. At the very least, it makes %%{foo}
syntactically ambiguous. Please pick a different syntax. %{} or %""
would work better, for example.

BTW, you may be able to support %"quoted strings" syntax where the
phrase inside quotes may contain spaces because we now have a general
purpose quoted string parser. However, I am not sure that quoted string
parser is compatible with the '%' prefix.


> +	  %%{..}	Staic text

"Static" is misspelled. It is also not very clear what "static" means
here, IMHO. Consider:

          %{text}     Constant text without spaces. The %{} decoration
                      is removed before passing the text to the helper.

And if you add support for spaces:

          %"text"     Constant text, possibly containing spaces. The ...


HTH,

Alex.
[prev in list] [next in list] [prev in thread] [next in thread]