Bhuvaneswaran wrote:
>> Wow. I tried to review this patch, but it's just too much churn for
>> me to
>> digest at once. For one thing, you didn't provide a log message, which
>> means I don't have that helpful hint alongside as I review. Secondly, it
>> appears you combined more than a handful of changes that each deserve
>> their
>> own distinct patch/review/commit into one giant patch.
>
>
> Yeah, it's my mistake. Now i'm reading
> http://subversion.tigris.org/hacking.html and i'll ensure not to
> repeat this mistake!
Excellent.
> So, i take no one else have thoroughly reviewed this patch and as
> suggested by cmpilato I'll break the changes up into (many) logical
> units. Here, i list those logical units. I'll submit the patches for
> each logical units seperately.
>
> 1) The script should call sys.exit(1) at the end of "except
> ImportError" block
> 2) Add a --max-items option
> 3) --revision could take a range, like the 'svn' client does
> 4) Lose --url, and add --url-template, which is a URL with replaceable
> fields
> 5) Come up with decent default values for options for which
> it makes sense to do so:
>
> --svn-path could default to nothing, meaning "look in $PATH"
> --rss-file could default to os.path.basename(self.repos_path) \
> + ".rss"
> --revision could default to the youngest revision in the repos
>
> 6) Validate input before trying to act on it
> 7) Lose --repos-path, and make it a command-line argument, not an option
Fantastic!
>> You changed this option name, but the behavior didn't really change at
>> all. So ... why the change at all? Also, I noticed that there are
>> actually two
>
>
> --url-template seem to be more appropriate for this option. For each
> rss item, we append revision tag to this value and include it in rss
> feed. So, this value on its own does not constitute the url (imo).
I see. Fair enough.
>> URLs in play in this script -- one that is associated with the feed name,
>> and one that's associated with the individual items. Why is only one of
>> those exposed via the options?
>
>
> Point. I will include it as another logical unit. I guess, --url
> option is more appropriate for this value.
Actually, in light of this, both --url and --url-template seem quite
inadequate in terms of descriptiveness. Maybe --item-url-template and
--feed-url (if that's appropriate ... I'm not sure what that other URL is
used for).
> Currently, the script does not seem to support windows system. IMO, we
> must give a fresh look and ensure it runs on windows systems. I dont
> want to include it as one of the logical unit until i complete
> existing ones.
Okey dokey.
> To conclude, i add following logical units:
>
> 8) Retain --url-template for feed item and include --url option for feed
> url
See previous naming suggestion.
> 9) Add following enhancements:
> * Consistent use of space-before-style
> * define cmd_output() function
> * Currently we have 3 rss stuff doing same thing. It's better to
> abstract as one
And:
* Either add Windows support, or remove existing Windows-y comments
and note in the usage message that the script assumes a Unix-ish
environment.
Looks good, Bhuvan. I await your slew of patches. :-)
--
C. Michael Pilato <cmpilato@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Received on Fri Jun 16 06:29:57 2006