Re: [PATCH V3 4/4] git-mw: Add preview subcommand into git mw.
- From
Matthieu Moy <matthieu.moy@grenoble-inp.fr>
- Date
- Jun 16, 2013, 20:39 UTC
- Message-ID
- <vpqr4g1eqnd.fsf@anie.imag.fr>
- In-Reply-To
- <1371349893-7789-5-git-send-email-benoit.person@ensimag.fr>
[ Just a quick look, no time for a detailed review ]
benoit.person@ensimag.fr writes:
> From: Benoit Person <benoit.person@ensimag.fr> > > Add the subcommand to 'git-mw.perl'.
That's already said in the Subject field.
> Add a new constant in GitMediawiki.pm 'HTTP_CODE_PAGE_NOT_FOUND'.
And this brings zero information compared to
> --- a/contrib/mw-to-git/GitMediawiki.pm > +++ b/contrib/mw-to-git/GitMediawiki.pm > - EMPTY HTTP_CODE_OK); > + EMPTY HTTP_CODE_OK HTTP_CODE_PAGE_NOT_FOUND);
I'd say your commit messages looks like a GNU-style changelog entry, which I do not consider a compliment ;-).
Still, after removing rendundant information, you may notice that the reader has no idea what "preview" is or does, and *why* it is a good thing to have it. How about:
" In the current state, a user of git-remote-mediawiki can edit the markup text locally, but has to push to the remote wiki to see how the page is rendered. Add a new "git mw preview" command that allows rendering the markup text on the remote wiki without actually doing any change on the wiki.
This uses MediaWiki's API to render the markup, and inserts the result in an actual HTML page from the wiki so that CSS be rendered properly. "
?
(The first paragraph answers "*why* did you do this?" and the second "*why* did you do it this way?")
(did I put enough emphasis on the "why"? ;-) )
> + # file_name argumeent is mandatory
argumeent -> argument
> + if (!defined $file_name) {
> + die "File not set, see `git mw help` \n";Perhaps "missing file argument"?
> + # Search all possibles mediawiki remotes
> + if (! $remote_url) {The "why" thing about commit message also applies to comments: when you start saying what you're doing in a comment, it usually means your code should be refactored.
Wouldn't it better to add a function here? The name of the function would probably carry the same information as the comment.
> + print {*STDERR} "do you want ? Use the -r option to specify the remote\n";Missing period (.).
> + }) or die "No response from distant mediawiki\n";
distant -> remote.
> + $template_content_id = Git::config('mediawiki.IDContent')
> + || $template_content_id;It would be cool to have also remote.<name>.IDContent or something like this in case you have several remotes with different div ids. But this can be added later.
Show 5 quoted lines
> @@ -41,6 +341,7 @@ usage: git mw <command> <args> > > git mw commands are: > help Display help information about git mw > + preview Parse and render local file into HTML
Space/tab mix after preview.
-- Matthieu Moy http://www-verimag.imag.fr/~moy/