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

DOC: Fixed README formatting #8859

Merged
merged 1 commit into from
Mar 28, 2017
Merged

DOC: Fixed README formatting #8859

merged 1 commit into from
Mar 28, 2017

Conversation

chittti
Copy link

@chittti chittti commented Mar 28, 2017

I've also changed the URL to https://api.travis-ci.org/numpy/numpy.svg?branch=master which is a direct link to the Travis build page's status.

@chittti chittti changed the title Fixed formatting MAINT: Fixed README formatting Mar 28, 2017
@juliantaylor
Copy link
Contributor

juliantaylor commented Mar 28, 2017

hi, please edit the actual commit title with git commit --amend, DOC: is the more appropriate prefix for this.
Also add [ci skip] to the commit message as this does not need to be tested.
Then push it with the -f flag to your branch

@eric-wieser
Copy link
Member

hi, please edit the actual commit title

This seems to be a recurring problem with first-time contributors - is there any way we can make this more obvious?

@chittti chittti changed the title MAINT: Fixed README formatting DOC: Fixed README formatting Mar 28, 2017
@chittti
Copy link
Author

chittti commented Mar 28, 2017

@juliantaylor Does this look okay?

@eric-wieser It was pretty clear in the contributing file. It was my mistake.

Maybe including a bold phrase at the very start of the contributing file? Something specifying that it is mandatory on both the PR title and all the commits, to follow the commit message convention, otherwise the PR will be rejected.

@juliantaylor
Copy link
Contributor

thanks, its [ci skip] without the hyphen though

@juliantaylor juliantaylor merged commit 31a8fd3 into numpy:master Mar 28, 2017
@rgommers
Copy link
Member

This seems to be a recurring problem with first-time contributors - is there any way we can make this more obvious?

To make it more obvious, we can include text or a checklist in the PR submission - that would be good for other things (docs, tests) as well.

For the rest, it is better not to treat this as mandatory. We've never done that, and first-time contributors often struggle with rebases. So it's better to just point it out for next time and go on with the merge. Unless the commit message is completely uninformative of course, but a missing DOC: is no deadly sin.

@eric-wieser
Copy link
Member

eric-wieser commented Mar 28, 2017

There are other options here to just letting it slide:

  • Use the merge-and-squash button on github. Do we care about not having true merges? Do we care more or less than not having prefixes?
  • Take advantage of the by-default "allow contributors to edit my PR" button. Perhaps a bot could be set up to amend the message then force-push upon request.

To make it more obvious, we can include text or a checklist in the PR submission - that would be good for other things (docs, tests) as well.

One thing to be clear about here - in other repos, like sphinx/sphinx, the PR message feels like a template (but seems not to be intended to be used as one) - we should make it clear that the template-text is instructions and not content to include in the PR.

HTML comments work in GFM, so we could include brief instructions in there

@rgommers
Copy link
Member

Use the merge-and-squash button on github. Do we care about not having true merges? Do we care more or less than not having prefixes?

Prefixes are not that important. Merge-and-squash may be fine for single commit PRs, but for reasonably structured sets of commits it's not a good idea.

Take advantage of the by-default "allow contributors to edit my PR" button.

That sounds fine, for maintainers who want to do that.

Perhaps a bot could be set up to amend the message then force-push upon request.

Hmm, most bots are annoying. Plus the work to implement it (there's no homu-like bot that does exactly what we want) doesn't seem worth it.

we should make it clear that the template-text is instructions and not content to include in the PR

Definitely, agreed

@chittti chittti deleted the patch-1 branch April 1, 2017 02:55
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

4 participants