Re: [PATCH] stage 2 of store url as fake store_url helper

On 12/02/2012 06:55 AM, Eliezer Croitoru wrote:

> Changed the storeUrl etc to storeId and store_id.

> +const char *HttpRequest::urlStoreId()
> +{
> + if (store_id)
> + return store_id;
> + else
> + return urlCanonical((HttpRequest*)this);
> +}

Please replace "urlStoreId" with "storeId". The method does not return
a URL.

Please either make the method constant or remove the cast to (HttpRequest*).

If you leave the cast in, use C++ const_cast<> instead of C-style cast
to make the intent clear.

Please document all new variables, functions, and methods (including
this one) using brief doxygen comments. I believe Squid style guide on
Squid wiki has more detailed instructions and examples.

>> Please search your patch for tabulation characters in sources and
>> replace the ones you added with spaces.

> Will do.

And please remove whitespace non-changes, such as those in the class
SquidConfig declaration.

> +NAME: store_id_program store_url_program
> +TYPE: wordlist
> +LOC: Config.Program.store_id
> +DEFAULT: none
> +DOC_START
> + Specify the location of the executable URL rewriter to use.

The new option does not rewrite URLs. It maps URLs to store entry IDs.
Please adjust documentation accordingly and check the text you have
copied for other discrepancies. BTW, I think it is better to just say
"same as directive_foo but applied to directive_bar" if the copied
description would be essentially the same as some existing directive
description.

> + // IF it contained valid entry for the old URL-rewrite helper protocol
> + debugs(85, DBG_IMPORTANT, "ERROR: store-URL helper returned invalid result code. Wrong helper? " << reply);
> + break;
> +
> + case HelperReply::BrokenHelper:
> + debugs(85, DBG_IMPORTANT, "ERROR: store-URL helper: " << reply << ", attempt #" << (store_id_fail_count+1) << " of 2");

This code still mentions store-URL helper and URL-rewrite.

> + storeAppendPrintf(sentry, "No StoreIds defined\n");

> + helperStats(sentry, storeIds, "StoreIds Statistics");

> + "because all store_Ids were busy: %d\n", n_bypassed);

Please be consistent in how you name the store ID mapping helper in
user-visible messages (and documentation).

Thank you,

Alex.
Received on Sun Dec 02 2012 - 17:56:33 MST