Re: [PATCH]proposal for a fake_strore_url helper wrapping.

On 09/09/2012 04:08 AM, Eliezer Croitoru wrote:
> I took the url_rewrite helper and used what ever seems needed to create
> a url_store_rewrite helper.
> I also added the needed variables for the next step and freeing them
> where and if needed.
> this is a more of a basic sketch.

Please do not mark sketch patches not meant for commit with a [PATCH]
prefix. [RFC] or no prefix would be better.

When you think these changes are ready for commit, please include a
proposed commit message. It is a good idea to do that even before the
last stage, of course -- it tells us what the patch is supposed to do.
"what ever seems needed to create a url_store_rewrite helper" is too
vague. If the code is committed with such a message, most of the
knowledge gained during recent discussions will be lost and somebody
would have to start from scratch when they want to understand how the
new feature works and, more importantly, why it should work that way.

Please do not forget to document all new members (using doxygen
comments) -- this is one of the mandatory requirements for patches. And,
as Amos pointed out, remove "for storeurl" marks -- everything you
change in this patch is for the storeurl feature (or at least it should
be) so the diff blobs themselves can be used as such a mark.

If this code is not sufficient to support the storeurl feature, I do not
think it should be committed to trunk -- it has no stand-alone value.
Commit it to your own branch so that you can maintain the progress
history and post incremental patches for discussion. When the feature is
completed, your branch can then be merged into trunk.

Thank you,

Alex.
Received on Mon Sep 10 2012 - 14:39:55 MDT