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

Fix #1981, improve ini-options help text #1983

Merged
merged 1 commit into from
Oct 4, 2016

Conversation

tomviner
Copy link
Contributor

@tomviner tomviner commented Oct 4, 2016

Fix for #1981

@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.949% when pulling c93a9e3 on fix-1981-improve-ini-options-help-text into fc02003 on master.

@RonnyPfannschmidt
Copy link
Member

@tomviner please check your git setup, you accidentally pushed your topic branch to the main repo

should we add a few lines for the workflow setup to the documentation (i recall making the same mistake a few weeks back)

@nicoddemus
Copy link
Member

please check your git setup, you accidentally pushed your topic branch to the main repo

Yes, it is recommended to open PRs from branches in your fork. FWIW the only drawback of opening PRs from branches in the main repository is that the CI services run twice. Other than that, there's really no long term drawback though.

should we add a few lines for the workflow setup to the documentation (i recall making the same mistake a few weeks back)

I believe the documentation is explicit about the recommendation of opening PRs from forks already... do you have something in mind @RonnyPfannschmidt?

I'm merging this, the py35-trial failure on Linux is not related, hopefully I'll be able to investigate it in #1979.

@nicoddemus nicoddemus merged commit d47ae79 into master Oct 4, 2016
@nicoddemus nicoddemus deleted the fix-1981-improve-ini-options-help-text branch October 4, 2016 15:43
@RonnyPfannschmidt
Copy link
Member

@nicoddemus i believe we should propose a configuration workflow,

recently i started to always clone from the pristine repos, and setting my fork as the push url for origin
perhaps a discussion on the ml makes sense

@nicoddemus
Copy link
Member

i believe we should propose a configuration workflow,

I see, I usually:

  1. Fork on GH, and clone my fork (this automatically sets my fork as origin).
  2. Use git remote add upstream <official repo url> to add a upstream remote pointing to the original repository, usually just to update my fork with upstream commits.

I think that's the recommended workflow in general when working on GH, although I don't really remember reading it specifically anywhere.

@tomviner
Copy link
Contributor Author

tomviner commented Oct 4, 2016

@tomviner please check your git setup, you accidentally pushed your topic branch to the main repo

should we add a few lines for the workflow setup to the documentation (i recall making the same mistake a few weeks back)

@RonnyPfannschmidt very sorry about that! The only reason it happened is because I made the change using the github web interface.

I would never normally create a branch on pytest-dev otherwise. And of course, it's only possible because I'm a pytest plugin maintainer, otherwise I'd have no rights to create branches here anyway.

@nicoddemus for what it's worth, my usual workflow for open source contributions is:

  1. Clone original repo, to look round / run the code base on my laptop. Assuming I need to make change:
  2. Click Fork on github
  3. Run a little shell function I have, called git-forked which moves the original repo to upstream and adds a new origin under my github username:
function git-forked {
    git remote rename origin upstream
    git remote add origin $(git ls-remote --get-url upstream | sed s/":[A-Za-z0-9-]\+\/"/":tomviner\/"/)
    git fetch --all
    git branch master -u origin/master
}

I'm sure that could be simplified though!

@nicoddemus
Copy link
Member

Hey @tomviner!

The only reason it happened is because I made the change using the github web interface.

Absolutely, I've done so in the past myself. 😁

@tomviner
Copy link
Contributor Author

tomviner commented Oct 4, 2016

image

@nicoddemus thanks for the clean up!

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.

4 participants