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

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Jun 17, 2013, 07:12 UTC
Message-ID: <vpq8v299pn6.fsf@anie.imag.fr>
In-Reply-To: <CAETqRChcu+N-2uNzL9_J6abR5fnduT5UVRif5NTQYOoxPTiRTw@mail.gmail.com>

Benoît Person <benoit.person@ensimag.fr> writes:

Show 12 quoted lines

> On 16 June 2013 22:18, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:
>> benoit.person@ensimag.fr writes:
>>
>>> 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.

Show 9 quoted lines

>> 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.

Show 8 quoted lines

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