git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: "Célestin Matte" <celestin.matte@ensimag.fr>
Cc: git@vger.kernel.org, benoit.person@ensimag.fr,
	matthieu.moy@grenoble-inp.fr
Subject: Re: [PATCH v2 02/22] git-remote-mediawiki: Use the Readonly module instead of the constant pragma
Date: Fri, 7 Jun 2013 23:23:25 -0400	[thread overview]
Message-ID: <20130608032324.GA20226@sigill.intra.peff.net> (raw)
In-Reply-To: <1370641344-4253-3-git-send-email-celestin.matte@ensimag.fr>

On Fri, Jun 07, 2013 at 11:42:03PM +0200, Célestin Matte wrote:

> diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl
> index 4893442..e60793a 100755
> --- a/contrib/mw-to-git/git-remote-mediawiki.perl
> +++ b/contrib/mw-to-git/git-remote-mediawiki.perl
> @@ -26,21 +26,21 @@ use IPC::Open2;
>  use Readonly;

What does this series apply on top of? The existing version in "master"
does not have "use Readonly" in it at all. The first version of your
series introduced that line, but here it is shown as an existing line.
Did you miss a commit when putting your patches together?

>  # Mediawiki filenames can contain forward slashes. This variable decides by which pattern they should be replaced
> -use constant SLASH_REPLACEMENT => "%2F";
> +Readonly my $SLASH_REPLACEMENT => "%2F";

What advantage does this have over "use constant"? I do not mind
following guidelines from perlcritic if they are a matter of style, but
in this case there is a cost: we now depend on the "Readonly" module,
which is not part of the standard distribution. I.e., users now have to
deal with installing an extra dependency. Is it worth it?

-Peff

  reply	other threads:[~2013-06-08  3:23 UTC|newest]

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-06-07 21:42 [PATCH v2 00/22] git-remote-mediawiki: Follow perlcritic's recommandations Célestin Matte
2013-06-07 21:42 ` [PATCH v2 01/22] git-remote-mediawiki: Replace :utf8 by :encoding(UTF-8) Célestin Matte
2013-06-07 21:42 ` [PATCH v2 02/22] git-remote-mediawiki: Use the Readonly module instead of the constant pragma Célestin Matte
2013-06-08  3:23   ` Jeff King [this message]
2013-06-08  8:51     ` Benoît Person
2013-06-08 13:01     ` Célestin Perdu
2013-06-08 17:31       ` Jeff King
2013-06-08 18:27         ` Matthieu Moy
2013-06-07 21:42 ` [PATCH v2 03/22] git-remote-mediawiki: Always end a subroutine with a return Célestin Matte
2013-06-07 21:42 ` [PATCH v2 04/22] git-remote-mediawiki: Move a variable declaration at the top of the code Célestin Matte
2013-06-07 21:42 ` [PATCH v2 05/22] git-remote-mediawiki: Change syntax of map calls Célestin Matte
2013-06-07 21:42 ` [PATCH v2 06/22] git-remote-mediawiki: Rewrite unclear line of instructions Célestin Matte
2013-06-07 21:42 ` [PATCH v2 07/22] git-remote-mediawiki: Change style of some regular expressions Célestin Matte
2013-06-08  0:12   ` Eric Sunshine
2013-06-07 21:42 ` [PATCH v2 08/22] git-remote-mediawiki: Add newline in the end of die() error messages Célestin Matte
2013-06-07 21:42 ` [PATCH v2 09/22] git-remote-mediawiki: Change the name of a variable Célestin Matte
2013-06-07 21:42 ` [PATCH v2 10/22] git-remote-mediawiki: Turn double-negated expressions into simple expressions Célestin Matte
2013-06-07 21:42 ` [PATCH v2 11/22] git-remote-mediawiki: Remove unused variable $entry Célestin Matte
2013-06-07 21:42 ` [PATCH v2 12/22] git-remote-mediawiki: Rename a variable ($last) which has the name of a keyword Célestin Matte
2013-06-07 21:42 ` [PATCH v2 13/22] git-remote-mediawiki: Assign a variable as undef and make proper indentation Célestin Matte
2013-06-07 21:42 ` [PATCH v2 14/22] git-remote-mediawiki: Check return value of open + remove import of unused open2 Célestin Matte
2013-06-08  0:14   ` Eric Sunshine
2013-06-08 15:54     ` Célestin Matte
2013-06-08 18:41       ` Matthieu Moy
2013-06-08 18:45         ` Célestin Matte
2013-06-08 19:04           ` Matthieu Moy
2013-06-09  4:52           ` Eric Sunshine
2013-06-07 21:42 ` [PATCH v2 15/22] git-remote-mediawiki: Put long code into a subroutine Célestin Matte
2013-06-08  0:27   ` Eric Sunshine
2013-06-07 21:42 ` [PATCH v2 16/22] git-remote-mediawiki: Modify strings for a better coding-style Célestin Matte
2013-06-08  0:39   ` Eric Sunshine
2013-06-08 20:32     ` Célestin Matte
2013-06-09  4:44       ` Eric Sunshine
2013-06-07 21:42 ` [PATCH v2 17/22] git-remote-mediawiki: Brace file handles for print for more clarity Célestin Matte
2013-06-08  0:42   ` Eric Sunshine
2013-06-07 21:42 ` [PATCH v2 18/22] git-remote-mediawiki: Replace "unless" statements with negated "if" statements Célestin Matte
2013-06-07 21:42 ` [PATCH v2 19/22] git-remote-mediawiki: Don't use quotes for empty strings Célestin Matte
2013-06-07 21:42 ` [PATCH v2 20/22] git-remote-mediawiki: Put non-trivial numeric values in constants Célestin Matte
2013-06-07 21:42 ` [PATCH v2 21/22] git-remote-mediawiki: Fix a typo ("mediwiki" instead of "mediawiki") Célestin Matte
2013-06-07 21:42 ` [PATCH v2 22/22] git-remote-mediawiki: Clearly rewrite double dereference Célestin Matte
2013-06-07 23:34   ` Eric Sunshine

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=20130608032324.GA20226@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=benoit.person@ensimag.fr \
    --cc=celestin.matte@ensimag.fr \
    --cc=git@vger.kernel.org \
    --cc=matthieu.moy@grenoble-inp.fr \
    /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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).