Re: [PATCH V3 4/4] git-mw: Add preview subcommand into git mw.

[Matthieu Moy <Matthieu.Moy@grenoble-inp.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.

> @@ -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/