Benoît Person <benoit.person@xxxxxxxxxx> writes:
> On 16 June 2013 22:18, Matthieu Moy <Matthieu.Moy@xxxxxxxxxxxxxxx> wrote:
>> benoit.person@xxxxxxxxxx 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.
>> 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.
>>> 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/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html