lazr-developers team mailing list archive

[Barry Warsaw <barry@xxxxxxxxxxxxx>]

On Jan 21, 2009, at 07:26 PM, Gary Poster wrote:

>Review: Approve code
>*merge-conditional (Edwin)
>
>This is fabulous!
>
>- I think --dry-run should imply --verbose

Good idea.  I've made sure that --dry-run implies at least one level of
verbosity.  The script honors more -v's if the user gives them.

>- Not specific to this check-in, but the test code depends on Py >= 2.5.  Is that
>  desirable?
>
>    return unittest.TestSuite((
>        doctest.DocFileSuite(
>            *doctest_files,
>            module_relative=True,
>            optionflags=doctest.NORMALIZE_WHITESPACE|doctest.ELLIPSIS),
>        ))

Fixed!

>- I first used ``./prepare.py lazr.foo``.  You can guess what that did.
>  Can we handle that better?  An error seems like the right thing to do.

Ouch.  Okay, I added a sanity check so that the name may not have any dots in
it.

>- the --help implies that I will have to type something in an editor, but I
>  don't. ("This physically edits the files, dropping you into the editor of
>  your choice (via $EDITOR) for any changes this script can't automate.")
>  Perhaps the instructions should be changed until this actually happens?
>  (In future revisions, I can imagine this opening up README and so on for
>  initial edits.)

Agreed.  At first I didn't think I could make all the changes
programmatically, but it turned out easier than I thought.

>- two tiny additional comments below

>> cre = re.compile(re.escape('yourpkg'))
>
>a niggle, but the "cre" name is not very informative. That's more noticeable
>later on in the code.

Ah yes, a Mailmanism leaking through.  Changed to 'regexp'.

>> def walk_and_replace(directory, name, options):
>>     """Walk the directory, looking for patterns in files to replace.
>>
>>     :param directory: The directory to begin walking.
>>     :type directory: string
>>     :param new_name: The new package's name.
>>     :type new_name: string
>
>Cut and paste error looks like: should be "name" not "new_name".

Good catch.  I've changed the argument to hack_file() to be consistent with
this.

Here's the incremental.  I'll respond to Edwin's review next.  Thanks for the
review Gary!

=== modified file 'prepare.py'
--- prepare.py	2009-01-20 21:34:28 +0000
+++ prepare.py	2009-01-22 02:56:51 +0000
@@ -32,7 +32,7 @@
 sys.path.insert(0, 'src')
 from lazr.yourpkg import __version__
 
-cre = re.compile(re.escape('yourpkg'))
+regexp = re.compile(re.escape('yourpkg'))
 
 
 def parse_arguments():
@@ -46,10 +46,8 @@
 %prog [options] name
 
 Hack the files in the lazr project template to reflect the name of the project
-you're creating.  This physically edits the files, dropping you into the
-editor of your choice (via $EDITOR) for any changes this script can't
-automate.  After running this script, you should verify all changes before
-committing them.""")
+you're creating.  After running this script, you should verify all changes
+before committing them.""")
     parser.add_option('-k', '--keep', action='store_true', default=False,
                       help='Keep this prepare.py script after completion.')
     parser.add_option('-v', '--verbose',
@@ -70,13 +68,13 @@
     return parser, options, arguments[0]
 
 
-def hack_file(src, new_name, options):
-    """Hack the contents of the file, essentially s/yourpkg/new_name/.
+def hack_file(src, name, options):
+    """Hack the contents of the file, essentially s/yourpkg/name/.
 
     :param src: The full path name of the file to hack.
     :type src: string
-    :param new_name: The new package's name.
-    :type new_name: string
+    :param name: The new package's name.
+    :type name: string
     :param options: The options instance.
     :type options: Options
     """
@@ -84,7 +82,7 @@
     total_substitution_count = 0
     with nested(open(src), open(dest, 'w')) as (in_file, out_file):
         for line in in_file:
-            substituted, substitution_count = cre.subn(new_name, line)
+            substituted, substitution_count = regexp.subn(name, line)
             out_file.write(substituted)
             total_substitution_count += substitution_count
     # Move the temporary file into place, unless dry-running.
@@ -102,8 +100,8 @@
 
     :param directory: The directory to begin walking.
     :type directory: string
-    :param new_name: The new package's name.
-    :type new_name: string
+    :param name: The new package's name.
+    :type name: string
     :param options: The options instance.
     :type options: Options
     """
@@ -126,7 +124,13 @@
 
 def main():
     parser, options, name = parse_arguments()
+    # Do some basic sanity checking on the name.
+    if name.count('.') > 0:
+        parser.error('Name may not have dots in it')
+    if options.dry_run:
+        options.verbosity = max(options.verbosity, 1)
     walk_and_replace('.', name, options)
+    # --dry-run implies at least one level of verbosity.
     if not options.keep:
         if options.verbosity > 0:
             print 'Removing prepare.py'

-- 
https://code.launchpad.net/~barry/lazr.yourpkg/prepare/+merge/2997
Your team LAZR Developers is subscribed to branch lp:lazr.yourpkg.