On 24/05/2013 7:38 a.m., Tsantilas Christos wrote:
This patch :
- adds support for quoted values in the entire squid.conf
- warn about or prohibit values that can no longer be interpreted as
either quoted strings or simple tokens
- support file:"path/to/file.name" syntax to load external configuration
files
- support macros in "double quoted" values
- support 'single quoted' values that do not expand macros
- replaces the strtok() calls with calls to the new
ConfigParser::NextToken()
- modify strtokFile to use new ConfigParser::NextToken()
- Add the new configuration_includes_quoted_values configuration option,
to control the squid parser behaviour. If set to on Squid will recognize
each "quoted string" after a configuration directive as a single parameter.
This is a Measurement Factory project
This is a big patch with multiple features combined into one massive
submission. So I am going to audit this in stages and lets sort out each
stage before moving on to issues wth the others.
Firstly, Design simplicity:
I do not believe we need to present such a wide range of potential
quoting and escaping mechanisms for squid.conf. We need to pick one
style which will be familiar to our administrators and ignore the other
styles. Double-quoting with \-escaping of " characters for strings is
very well known and the type of encoding most often requested.
IMO, we should drop the '-quoted string handling from this. It is the
first step on a slipery-slope toward arbitrary quoting styles and "why
not also add {-quoted strings or [-quoted strings?". Those are becoming
equally popular as people become familiar with YAML/SAS/JSON syntax.
Also, your design for MacroUser requires parser components to
register/unregister their Format class before parsing each "-quoted
string which might include macros. Which makes toggling support for it
on the quoting delimiter redundant and nothing more than extra confusion
for users.
==> Please drop '-quoted strings.
Secondly, multiple features in one patch:
The MacroUser system you are adding here is combining some, but not
enough, of the next step in the libformat project. Thank you for doing
that, but I do not think it appropriate to merge it into this whitespace
and quote handling patch. It was planned to be done as a followup once
the parser was updated in a way such as what this patch is doing.
Also by naming the class MacroUser you are adding a fourth term
(macro) to describe these %things, we already have %-tokens, %-tags,
%-format, and %-codes in use today. I think we should start to redux
those terms usage rather than expanding with another variation on the theme.
==> Please separate the MacroUser functionality out into a followup patch.
Amos
