From: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
To: benoit.person@ensimag.fr
Cc: git@vger.kernel.org, celestin.matte@ensimag.fr
Subject: Re: [PATCH/RFC] git-remote-mediawiki: new tool to preview local changes without pushing
Date: Sat, 08 Jun 2013 21:00:30 +0200 [thread overview]
Message-ID: <vpqzjv0tokx.fsf@anie.imag.fr> (raw)
In-Reply-To: <1370641831-9115-1-git-send-email-benoit.person@ensimag.fr> (benoit person's message of "Fri, 7 Jun 2013 23:50:31 +0200")
benoit.person@ensimag.fr writes:
> From: Benoit Person <benoit.person@ensimag.fr>
>
> The #7 issue on git-mediawiki's issue tracker [1] states that the ability to
> preview content without pushing would be a nice thing to have.
>
> This commit is a first attempt to achieve it. It adds a new git command,
> named `git mw`. This command accepts the subcommands `help` and `preview`
> for now.
Review could be easier if you have PATCH 1/2 introducing the command
with only "help" (essentially to check that the build system works), and
then focus on "preview".
I won't insist in splitting this particular commit, just take it as an
advice for future work.
> --- a/contrib/mw-to-git/Makefile
> +++ b/contrib/mw-to-git/Makefile
> @@ -5,13 +5,17 @@
> ## Build git-remote-mediawiki
>
> SCRIPT_PERL=git-remote-mediawiki.perl
> +SCRIPT_PERL_MW=git-mw.perl
Why do you need another variable? Just adding git-mw.perl to SCRIPT_PERL
should do it, no? (Well, probably the make SCRIPT_PERL=$(SCRIPT_PERL)
lacks quotes around the argument, but then you should fix that).
> +# Constants
> +# Mediawiki filenames can contain forward slashes. This variable decides by which pattern they should be replaced
> +Readonly::scalar $SLASH_REPLACEMENT => "%2F";
This one is copied from git-remote-mediawiki.perl, and is a
git-mediawiki specific thing, so it would not make sense to put it into
Git.pm. This starts convincing me that we should have a GitMediawiki.pm
or so, and both scripts should use it.
Another option would be to put everything in git-remote-mediawiki.perl,
and probably to have "git mw" as a wrapper around it (to avoid having to
type "git remote-mediawiki" which is a bit long).
But the script starts being a bit long, so I think it makes more sense
to split it. So I'd say
PATCH 1/3 : introduce "git mw"
PATCH 2/3 : move sharable code to a new module (and make sure it's
installed properly by "make install")
PATCH 3/3 : actually implement the preview feature
Perhaps others will have other/better advices.
> + # Auto-loading in browser
> + if ($autoload) {
> + open(my $browser, "-|:encoding(UTF-8)", "xdg-open ".$preview_file_name);
That could be read from Git's configuration, and default to xdg-open.
But you don't want to hardcode it in the middle of the code.
Also, why use open if you don't want to communicate with the process?
system would be sufficient, and won't need close.
Prefer passing an array of arguments, instead of concatenating strings
and then rely on command-line parsing to re-split it.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
next prev parent reply other threads:[~2013-06-08 19:00 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-06-07 21:50 [PATCH/RFC] git-remote-mediawiki: new tool to preview local changes without pushing benoit.person
2013-06-08 19:00 ` Matthieu Moy [this message]
2013-06-09 6:12 ` Jeff King
2013-06-09 6:08 ` Jeff King
2013-06-09 11:01 ` Matthieu Moy
2013-06-09 12:18 ` Benoît Person
2013-06-09 18:13 ` Jeff King
2013-06-09 12:35 ` Benoît Person
2013-06-09 14:37 ` Matthieu Moy
2013-06-09 18:32 ` Jeff King
2013-06-11 21:31 ` Benoît Person
2013-06-11 21:37 ` Jeff King
2013-06-12 6:55 ` Matthieu Moy
2013-06-12 8:55 ` Jeff King
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
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=vpqzjv0tokx.fsf@anie.imag.fr \
--to=matthieu.moy@grenoble-inp.fr \
--cc=benoit.person@ensimag.fr \
--cc=celestin.matte@ensimag.fr \
--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.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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 read-only IMAP folder(s) and NNTP newsgroup(s).