Re: [PATCH v1] Documentation: add setup instructions for Travis CI
On 13 Apr 2016, at 19:39, Junio C Hamano <gitster@pobox.com> wrote:
Show 26 quoted lines
> larsxschneider@gmail.com writes:
>
>> From: Lars Schneider <larsxschneider@gmail.com>
>>
>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
>> ---
>> Documentation/SubmittingPatches | 39 ++++++++++++++++++++++++++++++++++++---
>> 1 file changed, 36 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
>> index 98fc4cc..79e9b33 100644
>> --- a/Documentation/SubmittingPatches
>> +++ b/Documentation/SubmittingPatches
>> @@ -63,10 +63,43 @@ t/README for guidance.
>> When adding a new feature, make sure that you have new tests to show
>> the feature triggers the new behaviour when it should, and to show the
>> feature does not trigger when it shouldn't. Also make sure that the
>> +test suite passes after your commit.
>
> This is not a new issue, but it sounds as if you do not have to test
> if you are not doing a new shiny toy. Perhaps we should rephrase
> the last sentence a bit.
>
> After any code change, make sure that the entire test suite
> passes. After any documentation change, make sure that the
> resulting documentation set formats well.
> By the way, can you teach our Travis thing to check for the "make
> doc" failures?
Yes, expect a patch soonish.
Show 12 quoted lines
>
>> +We recommend to use our CI infrastructure to ensure your new feature
>> +passes all existing tests as well as your new tests on Linux, Mac, and
>> +(hopefully soon) Windows. Follow these steps for the initial setup:
>> +
>> + (1) Sign in to GitHub: https://github.com
>> + Please sign up first if you haven't already, it's free.
>
> Three issues:
>
> * None of the things utilized by the reader of this section looks
> like "our" infrastructure to me.
OK. How about "We recommend to use the Travis CI infrastructure..." instead?
> * The above makes it sound as if we recommend everybody to become
> GitHub customer, which is not really the case.
Agreed. I assume your suggested wording below would be fine?
> * This is overly long and deserves to be its own separate section,
> just like we have MUA specific hints, this is GitHub-Travis
> specific hints.
Show 24 quoted lines
> How about just saying
>
> If you have an account at GitHub (and you can get one for
> free to work on open source projects), you can use their
> Travis CI integration to test your changes on Linux, Mac,
> and (hopefully soon) Windows. See GitHub-Travis CI hints
> section for details.
>
> here, create a "GitHub-Travis CI hints" section just before "MUA
> specific hints" section, and move these numbered entries and the two
> paragraphs that follow to the new section. As the introductory text
> for the new section itself, it may make sense to repeat a rephrased
> version of the above there, e.g.
>
> --------------------------------------------------
> GitHub-Travis CI hints
>
> With an account at GitHub (you can get one for free to work
> on open source projects), you can use Travis CI to test your
> changes on Linux, Mac, and (hopefully soon) Windows.
>
> Follow these steps for the initial setup:
>
> (1) ...
Agreed. I also like Stefan's suggestion to move the CI stuff into a separate file. Any objections to this?
> I'd mildly prefer to leave "Please sign up first" line out
> of the first entry.
OK, I will remove it. My intention was to express that you need a GitHub account to use Travis CI.
Thanks for the review, Lars
Show 20 quoted lines
>> + ...
>> + (7) Enable Travis CI builds for your Git fork
>> +
>> +After the initial setup you can push your new feature branches to your
>> +Git fork on GitHub and check if they pass all tests here:
>> +https://travis-ci.org/<Your GitHub handle>/git/branches
>> +
>> +If they don't pass then they are marked "red". If that happens then
>> +click on the failing Travis CI job and scroll all the way down in the
>> +log. Find the line "<-- Click here to see detailed test output!" and
>> +click on the triangle next to the log line number to expand the detailed
>> +test output (example: https://travis-ci.org/git/git/jobs/122676187).
>> +Fix the problem and push an updated commit to your branch to ensure
>> +all tests pass.
>> +
>> +Do not forget to update the documentation to describe the updated
>> +behaviour of your new feature. It is currently a liberal mixture of US
>> and UK English norms for spelling and grammar, which is somewhat
>> unfortunate. A huge patch that touches the files all over the place
>> only to correct the inconsistency is not welcome, though. Potential
larsxschneider@gmail.com
