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

Add contributing guidelines to repo root #1

Closed
wants to merge 2 commits into from
Closed

Add contributing guidelines to repo root #1

wants to merge 2 commits into from

Conversation

dungpa
Copy link
Contributor

@dungpa dungpa commented Jan 13, 2015

This PR copies contributing guidelines from the wiki to repo root.
It gives better discoverability for contributing guidelines. Whenever a user opens a new issue/pull request, the contributing guidelines will show up.

See https://github.com/blog/1184-contributing-guidelines.

@tpetricek
Copy link
Contributor

+1 looks good to me!

@sergey-tihon
Copy link
Contributor

+1

@latkin
Copy link
Contributor

latkin commented Jan 14, 2015

Applied

@latkin latkin closed this Jan 14, 2015
@sergey-tihon
Copy link
Contributor

@latkin Why not just merge PR from GitHub UI? Now it looks like a canceled PR =(.

@forki
Copy link
Contributor

forki commented Jan 14, 2015

Yep I was just going to say this.
I'm suggesting you use the automatic pull request features.

There 2 possible ways:

  1. hit the merge button. (not always what you want)
  2. merge the pull request manually (why did you change the commiter?) and just push. Github will detect the merge and close the PR automatically

@latkin
Copy link
Contributor

latkin commented Jan 14, 2015

The "merge pull request" button does a no-fast-forward merge, which isn't great IMO... So far we have been doing merge --squash or rebase so that the history stays linear and each PR is a single commit.

In this particular case I wanted to move the commit to master and then merge to fsharp4 anyways, so pressing the button wouldn't have worked.

Is there a better way (besides the merge button) to close the PR so that it looks right?

@latkin
Copy link
Contributor

latkin commented Jan 14, 2015

I did merge it manually, but I did not see any change to this PR, so I just linked the commit and closed it.

git automatically updates the committer whenever you do rebase, etc.

@dungpa
Copy link
Contributor Author

dungpa commented Jan 14, 2015

Why do you prefer each PR as a single commit (just to understand the incentive)? Isn't it better to keep original commits so that we know how features are implemented incrementally?

@forki
Copy link
Contributor

forki commented Jan 14, 2015

I know this one is controversial, but the explicit merge-commit is not a bad thing because it makes the pull request visible in the git history. It allows us to find the discussion and additional information from the history/code.

@forki
Copy link
Contributor

forki commented Jan 14, 2015

regarding rebase and squash; most projects ask the author to clean up the pull request before they accept it.

@KevinRansom
Copy link
Member

I’m afraid it doesn’t work with our current codeflow. We our doing things that will make it possible eventually but right now that workflow doesn’t work for us.

Please let’s not boil the ocean, let us get a workflow that works well for us, and we can evolve it over time, as we find improvements that we like.

Kevin

From: Steffen Forkmann [mailto:[email protected]]
Sent: Wednesday, January 14, 2015 1:36 PM
To: Microsoft/visualfsharp
Subject: Re: [visualfsharp] Add contributing guidelines to repo root (#1)

I know this one is controversial, but the explicit merge-commit is not a bad thing because it makes the pull request visible in the git history. It allows us to find the discussion and additional information from the history/code.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1#issuecomment-69996578.

@forki
Copy link
Contributor

forki commented Jan 14, 2015

of course. we just trying to understand.

@latkin
Copy link
Contributor

latkin commented Jan 14, 2015

Merge vs rebase is indeed religious war territory, don't want this to devolve into that. Single commit per PR is not uncommon - see here, here.

I think I should have just added "Closes #1" to the message, and that would have done the auto-linkup. Totally agree that it's nice having everything linked up properly. Apologies I didn't figure that our initially, we are still GitHub n00bs.

@latkin
Copy link
Contributor

latkin commented Jan 14, 2015

Basically,
noidea

@forki
Copy link
Contributor

forki commented Jan 14, 2015

A gif. Finally ;-)

All is well

@dungpa dungpa deleted the contributing-guideline branch January 14, 2015 23:56
OmarTawfik pushed a commit that referenced this pull request Dec 3, 2015
Tweaks for internal build
dsyme pushed a commit that referenced this pull request May 26, 2016
KevinRansom pushed a commit that referenced this pull request Jun 9, 2016
dsyme pushed a commit that referenced this pull request Jul 23, 2016
liboz pushed a commit to liboz/visualfsharp that referenced this pull request Oct 5, 2016
Adding ListEnumerable, Choose, and some cleanup
@vasily-kirichenko vasily-kirichenko mentioned this pull request Dec 19, 2016
1 task
saul pushed a commit to saul/fsharp that referenced this pull request Apr 22, 2019
nojaf referenced this pull request in nojaf/fsharp Jul 20, 2022
Introduce TupleTypeSegment to SynType.Tuple.
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.

6 participants