From: Matthieu Moy <Matthieu.Moy@grenoble-inp•fr>
To: Pavel Volek <Pavel.Volek@ensimag•imag.fr>
Cc: git@vger•kernel.org, Volek Pavel <me@pavelvolek•cz>,
NGUYEN Kim Thuat <Kim-Thuat.Nguyen@ensimag•imag.fr>,
ROUCHER IGLESIAS Javier <roucherj@ensimag•imag.fr>
Subject: Re: [PATCHv1] git-remote-mediawiki: import "File:" attachments
Date: Fri, 08 Jun 2012 16:42:53 +0200 [thread overview]
Message-ID: <vpqmx4drhjm.fsf@bauges.imag.fr> (raw)
In-Reply-To: <1339165376-20267-1-git-send-email-Pavel.Volek@ensimag.imag.fr> (Pavel Volek's message of "Fri, 8 Jun 2012 16:22:56 +0200")
Pavel Volek <Pavel.Volek@ensimag•imag.fr> writes:
> --- a/contrib/mw-to-git/git-remote-mediawiki
> +++ b/contrib/mw-to-git/git-remote-mediawiki
If the patch adds support for [[File:...]], then it should remove/adapt
the comment at the top of the file :
# Known limitations:
#
# - Only wiki pages are managed, no support for [[File:...]]
# attachments.
> @@ -212,59 +212,230 @@ sub get_mw_pages {
> my $user_defined;
> if (@tracked_pages) {
> $user_defined = 1;
> - # The user provided a list of pages titles, but we
> - # still need to query the API to get the page IDs.
> -
> - my @some_pages = @tracked_pages;
> - while (@some_pages) {
> - my $last = 50;
> - if ($#some_pages < $last) {
> - $last = $#some_pages;
> - }
> - my @slice = @some_pages[0..$last];
> - get_mw_first_pages(\@slice, \%pages);
> - @some_pages = @some_pages[51..$#some_pages];
> - }
> + get_mw_tracked_pages(\%pages);
> }
> if (@tracked_categories) {
> $user_defined = 1;
> - foreach my $category (@tracked_categories) {
> - if (index($category, ':') < 0) {
> - # Mediawiki requires the Category
> - # prefix, but let's not force the user
> - # to specify it.
> - $category = "Category:" . $category;
> - }
> - my $mw_pages = $mediawiki->list( {
> - action => 'query',
> - list => 'categorymembers',
> - cmtitle => $category,
> - cmlimit => 'max' } )
> - || die $mediawiki->{error}->{code} . ': ' . $mediawiki->{error}->{details};
> - foreach my $page (@{$mw_pages}) {
> - $pages{$page->{title}} = $page;
> - }
> - }
> + get_mw_tracked_categories(\%pages);
> }
> if (!$user_defined) {
> - # No user-provided list, get the list of pages from
> - # the API.
> - my $mw_pages = $mediawiki->list({
> - action => 'query',
> - list => 'allpages',
> - aplimit => 500,
> - });
> - if (!defined($mw_pages)) {
> - print STDERR "fatal: could not get the list of wiki pages.\n";
> - print STDERR "fatal: '$url' does not appear to be a mediawiki\n";
> - print STDERR "fatal: make sure '$url/api.php' is a valid page.\n";
> - exit 1;
> + get_mw_all_pages(\%pages);
> + }
> + return values(%pages);
> +}
The refactoring is welcome, but it would have been better to make it in
a separate patch. The patch as you made it is long and hard to review,
because it combines several new features, and refactoring.
> +sub get_mw_tracked_pages {
> + my $pages = shift;
> + # The user provided a list of pages titles, but we
> + # still need to query the API to get the page IDs.
> + my @some_pages = @tracked_pages;
> + while (@some_pages) {
> + my $last = 50;
> + if ($#some_pages < $last) {
> + $last = $#some_pages;
> + }
> + my @slice = @some_pages[0..$last];
> + get_mw_first_pages(\@slice, \%{$pages});
> + @some_pages = @some_pages[51..$#some_pages];
> + }
> +
> + # Get pages of related media files.
> + get_mw_linked_mediapages(\@tracked_pages, \%{$pages});
[...]
> +sub get_mw_linked_mediapages {
This is a nice feature, but I think it deserves to be configurable (if
the user explicitely specified one page, it actually seems strange to
import all the files it links to by default). Also, it should be
mentionned in the commit message.
Shouldn't the function be named get_mw_linked_mediafiles instead? In
general, the wording "media page" is used in many places in the code,
I prefer "media file" which is unambiguous.
> +sub get_mw_medafile_for_mediapage_revision {
medafile -> mediafile ?
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
next prev parent reply other threads:[~2012-06-08 14:43 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-06-08 14:22 [PATCHv1] git-remote-mediawiki: import "File:" attachments Pavel Volek
2012-06-08 14:42 ` Matthieu Moy [this message]
2012-06-08 16:20 ` Simon.Cathebras
2012-06-08 17:03 ` konglu
2012-06-08 23:24 ` Simon.Cathebras
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=vpqmx4drhjm.fsf@bauges.imag.fr \
--to=matthieu.moy@grenoble-inp$(echo .)fr \
--cc=Kim-Thuat.Nguyen@ensimag$(echo .)imag.fr \
--cc=Pavel.Volek@ensimag$(echo .)imag.fr \
--cc=git@vger$(echo .)kernel.org \
--cc=me@pavelvolek$(echo .)cz \
--cc=roucherj@ensimag$(echo .)imag.fr \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
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