You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Bhuvaneswaran Arumugam <bh...@collab.net> on 2006/06/16 05:32:04 UTC
[PATCH] svn2rss.py: Improve error handling and add --max-itemsoption
Hello,
I have attached a patch for svn2rss.py to improve error handling and add
--max-items option.
[[[
Improve error handling and add --max-items option
Patch by: Bhuvaneswaran <bh...@collab.net>
Suggested by: cmpilato
* svn2rss.py
add sys.exit(1) in 'except ImportError' block
Add --max-items option
(__init__): If max_items is not set, set it to 20
]]]
--
Regards,
Bhuvaneswaran
Re: [PATCH] svn2rss.py: Improve error handling and add --max-itemsoption
Posted by "C. Michael Pilato" <cm...@collab.net>.
Bhuvaneswaran wrote:
>>Here, you should validate the input a bit:
>>
>> try:
>> max_items = arg
>> except ValueError, msg:
>> usage_and_exit("Invalid value '%s' for --max-items." % (arg))
>> if max_items < 1:
>> usage_and_exit("Value for --max-items must be a positive integer.")
>
> Yeah, i did the change except a trivial correction. The correction is as
> follows:
>
> max_items = int(arg)
>
> IMO, it serves the purpose better.
Ahem. Yes, of course. (That's what I get for freetyping into the mail
instead of actually tweaking the script and testing.)
> [[[
> Add --max-items option
>
> Patch by: Bhuvaneswaran <bh...@collab.net>
> Suggested by: cmpilato
>
> * svn2rss.py
> (__init__): Add --max-items option
> ]]]
Looks good. Committed in r20146.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: [PATCH] svn2rss.py: Improve error handling and add--max-itemsoption
Posted by Bhuvaneswaran Arumugam <bh...@collab.net>.
Hello,
I've implemented the proposed changes and attached the patch. Please see
my comment against each remarks.
> > Index: svn2rss.py
>
> Note for the future -- always generate your patches from the top of the
> Subversion tree.
>
ok.
> Now, this is odd. Why not go ahead and set max_items = 20?
>
Yes, i did.
> Here, you should validate the input a bit:
>
> try:
> max_items = arg
> except ValueError, msg:
> usage_and_exit("Invalid value '%s' for --max-items." % (arg))
> if max_items < 1:
> usage_and_exit("Value for --max-items must be a positive integer.")
>
Yeah, i did the change except a trivial correction. The correction is as
follows:
max_items = int(arg)
IMO, it serves the purpose better.
>
> self.max_items = max_items
>
Ok, i did this. Here is the log message:
[[[
Add --max-items option
Patch by: Bhuvaneswaran <bh...@collab.net>
Suggested by: cmpilato
* svn2rss.py
(__init__): Add --max-items option
]]]
--
Regards,
Bhuvaneswaran
Re: [PATCH] svn2rss.py: Improve error handling and add --max-itemsoption
Posted by "C. Michael Pilato" <cm...@collab.net>.
Bhuvaneswaran Arumugam wrote:
> Hello,
>
> I have attached a patch for svn2rss.py to improve error handling and add
> --max-items option.
>
> [[[
> Improve error handling and add --max-items option
>
> Patch by: Bhuvaneswaran <bh...@collab.net>
> Suggested by: cmpilato
>
> * svn2rss.py
> add sys.exit(1) in 'except ImportError' block
> Add --max-items option
> (__init__): If max_items is not set, set it to 20
> ]]]
Cool. You are still tackling two different issues in one change, but I know
how hard it is to resist piggybacking a one-liner onto something else.
Decent log message, though I suppose all the items listed are technically
part of __init__.
> Index: svn2rss.py
Note for the future -- always generate your patches from the top of the
Subversion tree.
> ===================================================================
> --- svn2rss.py (revision 20142)
> +++ svn2rss.py (working copy)
> @@ -8,6 +8,7 @@
> -p | --repos-path= svn repository to generate RSS 2.0 feed
> -u | --url= link to appear in the rss item
> -f | --rss-file= filename to store the rss feed
> + -m | --max-items= maximum items to store in the rss feed
>
> Generates a RSS 2.0 file containing commit information. Once the
> maximum number of items is reached, older elements are removed. The
> @@ -33,6 +34,7 @@
> print >> sys.stderr, "PyRSS2Gen can be downloaded from:"
> print >> sys.stderr, "http://www.dalkescientific.com/Python/PyRSS2Gen.html"
> print >> sys.stderr, ""
> + sys.exit(1)
Here's that change that technically should be in its own commit. I'm going
to go ahead and commit that part of it.
> @@ -41,16 +43,19 @@
> usage(sys.stderr)
> sys.exit(2)
> try:
> - opts, args = getopt.gnu_getopt(sys.argv[1:],"hP:r:p:u:f:", [
> + opts, args = getopt.gnu_getopt(sys.argv[1:],"hP:r:p:u:f:m:", [
> "help", "svn-path=",
> "revision=",
> "repos-path=", "url=",
> - "rss-file="])
> + "rss-file=",
> + "max-items="])
> except getopt.GetoptError, msg:
> print >> sys.stderr, msg
> usage(sys.stderr)
> sys.exit(2)
>
> +max_items = None
> +
Now, this is odd. Why not go ahead and set max_items = 20?
> for opt, arg in opts:
> if opt in ("-h", "--help"):
> usage(sys.stdout)
> @@ -65,15 +70,22 @@
> url = arg
> elif opt in ("-f", "--rss-file"):
> rss_file = arg
> + elif opt in ("-m", "--max-items"):
> + max_items = arg
Here, you should validate the input a bit:
try:
max_items = arg
except ValueError, msg:
usage_and_exit("Invalid value '%s' for --max-items." % (arg))
if max_items < 1:
usage_and_exit("Value for --max-items must be a positive integer.")
> class SVN2RSS:
> - def __init__(self, svn_path, revision, repos_path, url, rss_file):
> - self.max_items = 20
> + def __init__(self, svn_path, revision, repos_path, url, rss_file, max_items):
> self.svn_path = svn_path
> self.revision = revision
> self.repos_path = repos_path
> self.url = url
> self.rss_file = rss_file
> +
> + if (max_items == None):
> + self.max_items = 20
> + else:
> + self.max_items = int(max_items)
Ah, so you made the *class* have a notion of a default. You don't permit
max_items to be an optional input, and you document that None is allowed, so
I'd recommend doing as I said about (setting the default outside the class),
and here just a simple:
self.max_items = max_items
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand