Thanks Nicolas, your feedback is very helpful.<div><br></div><div>I will circle around on your comments and resubmit shortly.</div><div><br></div><div>Cheers</div><div>David<br><div><br><div class="gmail_quote">On Tue, Jun 8, 2010 at 10:13 PM, Nicolas Dumazet <span dir="ltr"><<a href="mailto:nicdumz@gmail.com">nicdumz@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">Hello David, and welcome aboard!<br>
<br>
2010/6/7 David Mitchell <<a href="mailto:david.mitchell@telogis.com">david.mitchell@telogis.com</a>>:<br>
<div class="im">> # HG changeset patch<br>
> # User David Mitchell <<a href="mailto:david.mitchell@telogis.com">david.mitchell@telogis.com</a>><br>
> # Date 1275870701 -43200<br>
> # Branch stable<br>
> # Node ID a26fd6d9d9f60da713f64a92f5713d724c94adef<br>
> # Parent d3ebb1a0bc49559e1e41d37f69c2afa06722563e<br>
> Add option to status to see changes in all subrepos.<br>
> Only used by status command so it has no impact on the way commits are done,<br>
> and therefore no effect on the default commit status comment.<br>
> Doesn't show which changes are files in subrepos and which are the top level<br>
> repo.<br>
<br>
</div>Thanks for the inline (patchbombed) patch. That's the proper way to<br>
ask for review here!<br>
<br>
I like the idea, but I will however not look too much at the code<br>
because... it is not tested =)<br>
If you want people to review seriously the patch, you might want to<br>
resend a version with a few tests for the new feature.<br>
<br>
Because it changes command semantics, it will also break a few tests,<br>
and in particular test-debugcomplete. It's okay with us, but you will<br>
need to submit along the changes to the output. (easy trick: "python<br>
run-tests.py -i test-debugcomplete" will run interactively the test<br>
and ask you if changes are the ones you want, before updating for you<br>
the expected output.) In general, running the full test suite (make<br>
tests) before sending in patches do help =)<br>
<br>
As for the log message, we usually use the following convention:<br>
"""<br>
component: very short description<br>
<br>
longer description after a linebreak. This is a long description<br>
and it is wrapped at 80 chars.<br>
"""<br>
Here for example, we'd expect "status: add --subrepo option to list<br>
changes in subrepos\n\netc..."<br>
<div class="im"><br>
> diff -r d3ebb1a0bc49 -r a26fd6d9d9f6 mercurial/commands.py<br>
</div>[...]<br>
<div class="im">> def status(self, node1='.', node2=None, match=None,<br>
> - ignored=False, clean=False, unknown=False):<br>
> + ignored=False, clean=False, unknown=False, showSubs=False):<br>
> """return status of files between two nodes or node and working<br>
> directory<br>
<br>
</div>We are not too fond of camelCase here ( have a peek at<br>
<a href="http://mercurial.selenic.com/wiki/BasicCodingStyle" target="_blank">http://mercurial.selenic.com/wiki/BasicCodingStyle</a> and at the more<br>
generic <a href="http://mercurial.selenic.com/wiki/ContributingChanges" target="_blank">http://mercurial.selenic.com/wiki/ContributingChanges</a><br>
explaining why we get so picky when reviewing patches, and what one<br>
should know to get through more easily :p ).<br>
"subrepo" might be enough, or "showsubrepo" if you feel that you need<br>
to precise more the semantics.<br>
<br>
Regards,<br>
<font color="#888888"><br>
--<br>
Nicolas Dumazet — NicDumZ<br>
</font></blockquote></div><br></div></div>