Re: [PATCH V3 1/4] git-mw: Introduction of GitMediawiki.pm

Benoît Person [off-list ref] writes:

On 16 June 2013 22:18, Matthieu Moy [off-list ref] wrote:

quoted

benoit.person@ensimag.fr writes:

quoted

changes from the V2:
  - Add a way to test, without installation, code that uses GitMediawiki.pm.

This still needs to be documented, even very quickly, somewhere in the
code (e.g a comment in the Makefile).

Well, I think I will have to re-read some docs (and some earlier
reviews) about what to write in commit messages, in the emails, in the
code as comments and in the documentation ... I am just totally lost
right now :/ .

Don't worry, reviews are meant to improve your code (present and
future), not to blame you ;-).

Just think about what you expect as a user or developer. Would you run
"git log Makefile" or "git blame Makefile" to know how to use the
Makefile? Commit messages are primarily meant for reviewers ("here's
some code, and here's why it's good and why you should merge it"), and
can be very useful when bisecting a regression or blaming a source file.

Right now, git-remote-mediawiki has only little doc, and the user manual
is hosted on a GitHub wiki, not in the source. So there's no ideal place 
to say how to use the tool as a developer, but a comment in the Makefile
should be OK.

quoted

Also, it seems to be only part of the solution. With your change, from
contrib/mw-to-git/ and after running only "make",

./git-mw takes the installed version of GitMediawiki.pm in priority

../../bin-wrappers/git takes the installed version of git-mw only (i.e.
does not know "git mw" if "make install" hasn't been ran).

Same thing as the documentation point, I think I am a bit lost in that
whole thing. I will re-look into it for the next version :/ .

In short, the include path should contain both the *.pm file and the
git-<foo> ones.

quoted

quoted

 perlcritic:
-     perlcritic -2 *.perl
+     perlcritic -2 *.perl
\ No newline at end of file

Please, avoid these whitespace-only changes. They create noise during
review, and more potential conflicts.

For that one, I don't know why git assumes there is a change in it :

I think you removed a newline from the end of the file. It's usually
considered good practice to have this trailing newline (e.g. so that
"cat file" in a terminal doesn't put your prompt after the last line).
IIRC, it's actually required to call the file a "text file" according to
POSIX.

I will look into that for the next version ...

In any case, using "git add -p" and if needed its "s" command avoids
introducing unwanted things in the commit.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/