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/

On 17 June 2013 09:12, Matthieu Moy [off-list ref] wrote:

quoted

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.

The fact is, for now, is there a way to test changes in
git-remote-mediawiki.perl without 'make install'-ing it ? I could not find one

So maybe in the "build-perl-script" of the toplevel Makefile we could add
something copying the script at the toplevel ?

And in GitMediawiki's Makefile, we let everything stay as is : copying *.pm
into /perl/blib/lib when building and copying it in installdir when installing ?

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.

That catch oO, thanks for the explanations.
From my point of view, this could definitely be improved from:

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

to something like that:

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

which gives more insights into why the line is considered "edited".

Benoit Person