Re: [PATCH] Refactoring url to original url (storeurl step 1)

On 11/27/2012 11:37 PM, Eliezer Croitoru wrote:
> The patch is refactoring of two variables:
> mem_object->url into mem_obj->original_url
> Store_entry->url into Store_entry->originalUrl
>
> Both are named inline with other variables in their scope.
> This is the first step towards Store_url completion.

Please consider documenting the renamed method and data member by adding
a brief doxygen comment for them. After spending so much time with this
code, you probably know what these members are! :-)

The new comment in StoreMetaURL::checkConsistency() does not belong to
this patch, IMO. If you insist on keeping it, then please spellcheck
"rewritted".

Overall, I think this change is a very important step, but it should not
be committed until we have other steps (or at all):

* The primary point of this change is to track cases where the rewritten
"store URL" should be used instead of the "original URL". If we commit
this patch now, we will not longer be able to track those cases when
reviewing your future patches.

* There is no need to create hundreds of conflicts with pending and
future changes for the renaming sake alone. At the very end of the
project, the renaming-only changes can be removed OR, if we decide that
they should stay, there will be a compelling reason to keep them -- we
would get the store_url feature as a payoff.

Thank you,

Alex.
Received on Wed Nov 28 2012 - 16:36:53 MST