Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added build status icon to readme #617

Closed
wants to merge 1 commit into from

Conversation

samuelstjean
Copy link
Contributor

Looks so nice! As for why it's useful since pull request are already all passing build test before merging, think of what happened with cython 0.22 giving failures everywhere for no particular reason. Same thing could happen to the master branch if a dependency becomes borked up/change it's API between two master merge, and nobody would likely notice it. A seemingly unlucky random user could clone a non working version and claim the whole thing to be broken due to pure (un)luck.

Which brings me to this little icon. Well, it only makes sense if the build triggers automatically from time to time or for each merge, which would make it not cover the case stated here. But anyway, it still useful for first time users to see the master branch is working.

At the same time, why is the project's title in all caps? It always bug me, since it's the only place in the readme where it's like that, as opposed to having the first letter capitalized and everything else not capitalized (is there an english word for non-capitalized letters? Just curious).

@stefanv
Copy link
Contributor

stefanv commented Mar 30, 2015

I'm not sure whether this is a good idea--Travis is quite unstable at times, so build faults (not the project's fault) will now be reported very visibly.

@arokem
Copy link
Contributor

arokem commented Mar 30, 2015

To answer your question, I think that non-capitalized letters are referred to as "lower-case". I think it's OK to add this icon, but is not of huge value. On the one hand, if a travis build is broken, we'll usually rerun it pretty soon. On the other hand, it is extremely rare for the master to be broken. We usually manage to mitigate that on branches/PRs before it hits master.

@arokem
Copy link
Contributor

arokem commented Mar 30, 2015

Which is to say: users should assume that master is not broken

@samuelstjean
Copy link
Contributor Author

A lot of other projects like to boast by using this, but it's mostly cosmestic. The interesting question is if the picture (and related build) is only triggerred on merge on the master branch (which means that the build will always pass according to how things are usually going) or that it might break from time to time.

And thanks for the english lesson, apparently it just slipped out of my mind that lower and upper case letters could be used for that.

@arokem
Copy link
Contributor

arokem commented Mar 30, 2015

Yes - I think that a build is triggered by the merge commit on master.
Ideally, we wouldn't merge anything before we are sure that it's not
breaking master, so we are usually pretty sure that this should always look
just fine. However, as we've seen, travis might sometimes time-out, or
flake out for other reasons, that aren't our fault. So - as Stefan
suggested - this would usually just indicate some Travis malfunction,
rather than some useful information about the project, because we are
usually pretty good about double-checking that everything passes before we
merge into master.

On Sun, Mar 29, 2015 at 9:34 PM, Samuel St-Jean notifications@github.com
wrote:

A lot of other projects like to boast by using this, but it's mostly
cosmestic. The interesting question is if the picture (and related build)
is only triggerred on merge on the master branch (which means that the
build will always pass according to how things are usually going) or that
it might break from time to time.

And thanks for the english lesson, apparently it just slipped out of my
mind that lower and upper case letters could be used for that.


Reply to this email directly or view it on GitHub
#617 (comment).

@matthew-brett
Copy link
Contributor

I agree with Stefan and Ariel - too much noise in travis to put this anywhere prominent - and I agree - we're not missing build failures on travis, as far as I can see.

@Garyfallidis
Copy link
Contributor

Let's keep it simple. Thanx @samuelstjean for the suggestion. But it seems it is better to not have this. I am proposing to close this PR. Is that okay?

@samuelstjean
Copy link
Contributor Author

oh too bad for the icon then. What about having DIPY all caps on top of
the readme? Am I the only one wanting to chnage it back to Dipy for
apparent consistency?

Le 2015-03-30 08:16, Eleftherios Garyfallidis a écrit :

Let's keep it simple. Thanx @samuelstjean
https://github.com/samuelstjean. I am proposing to close this PR. Is
that okay?


Reply to this email directly or view it on GitHub
#617 (comment).

@Garyfallidis
Copy link
Contributor

All caps or Dipy is fine. No reason to have consistency on that. I found using all capitals reads better in papers and Dipy reads better on blogs and websites. I like with all capitals on the readme file.

@samuelstjean samuelstjean deleted the patch-1 branch March 30, 2015 14:51
@samuelstjean samuelstjean mentioned this pull request Oct 15, 2015
@arokem
Copy link
Contributor

arokem commented Oct 15, 2015

I'm still -1 on this. Breakage like the one in #731 is rather rare, and users usually know when they upgrade something like numpy on their system. In this case, we were caught unawares, but only for a very small number of hours, and it was fixed within a rather short time! So the system seems to be working just fine.

@Garyfallidis
Copy link
Contributor

-1 here too

@samuelstjean
Copy link
Contributor Author

Oh well, I still think it looks nice and give a sense of polish for people stumbling upon this repo, but whatever.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants