All of lore.kernel.org
 help / color / mirror / Atom feed
From: Pete Wyckoff <pw@padd.com>
To: "Eric S. Raymond" <esr@thyrsus.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] Python scripts audited for minimum compatible version and checks added.
Date: Mon, 24 Dec 2012 08:36:49 -0500	[thread overview]
Message-ID: <20121224133649.GA1400@padd.com> (raw)
In-Reply-To: <20121220141855.05DAA44105@snark.thyrsus.com>

esr@thyrsus.com wrote on Thu, 20 Dec 2012 09:13 -0500:
> diff --git a/git-p4.py b/git-p4.py
> index 551aec9..ec060b4 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -12,6 +12,11 @@ import optparse, sys, os, marshal, subprocess, shelve
>  import tempfile, getopt, os.path, time, platform
>  import re, shutil
>  
> +if sys.hexversion < 0x02040000:
> +    # The limiter is the subprocess module
> +    sys.stderr.write("git-p4.py: requires Python 2.4 or later.")
> +    sys.exit(1)
> +
>  verbose = False

If 2.3 does not have the subprocess module, this script will fail
at the import, and not run your version test.

All the uses of sys.stderr.write() should probably include a
newline.  Presumably you used write instead of print to avoid
2to3 differences.

The name of this particular script, as users would type it, is
"git p4"; no dash and no ".py".

Many of your changes have these three problems; I just picked on
my favorite one.

> diff --git a/git-remote-testgit.py b/git-remote-testgit.py
> index 5f3ebd2..22d2eb6 100644
> --- a/git-remote-testgit.py
> +++ b/git-remote-testgit.py
> @@ -31,6 +31,11 @@ from git_remote_helpers.git.exporter import GitExporter
>  from git_remote_helpers.git.importer import GitImporter
>  from git_remote_helpers.git.non_local import NonLocalGit
>  
> +if sys.hexversion < 0x01050200:
> +    # os.makedirs() is the limiter
> +    sys.stderr.write("git-remote-testgit.py: requires Python 1.5.2 or later.")
> +    sys.exit(1)
> +

This one, though, is a bit of a lie because git_remote_helpers
needs 2.4, and you add that version enforcement in the library.

I assume what you're trying to do here is to make the
version-related failures more explicit, rather than have users
parse an ImportError traceback, e.g.  That seems somewhat useful
for people with ancient installs.

But what about the high-end of the version range?  I'm pretty
sure most of these scripts will throw syntax errors on >= 3.0,
how should we catch that before users see it?

		-- Pete

  parent reply	other threads:[~2012-12-24 13:37 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-12-20 14:13 [PATCH] Python scripts audited for minimum compatible version and checks added Eric S. Raymond
2012-12-20 14:48 ` Jeff King
2012-12-20 15:02   ` Eric S. Raymond
2012-12-20 18:25     ` Junio C Hamano
2012-12-21  3:38       ` Junio C Hamano
2012-12-24  4:57         ` Junio C Hamano
2012-12-24  5:21           ` Eric S. Raymond
2012-12-21 15:58 ` Manlio Perillo
2012-12-24 13:36 ` Pete Wyckoff [this message]
2012-12-24 15:32   ` Eric S. Raymond
2012-12-24 19:12     ` Junio C Hamano
2012-12-24 19:52       ` Eric S. Raymond

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20121224133649.GA1400@padd.com \
    --to=pw@padd.com \
    --cc=esr@thyrsus.com \
    --cc=git@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.