Skip to content

*: make contribution more newbie friendly#3102

Merged
shenli merged 4 commits into
pingcap:masterfrom
zhexuany:rewrite_contribution_guid
Apr 27, 2017
Merged

*: make contribution more newbie friendly#3102
shenli merged 4 commits into
pingcap:masterfrom
zhexuany:rewrite_contribution_guid

Conversation

@zhexuany
Copy link
Copy Markdown
Contributor

@zhexuany zhexuany commented Apr 20, 2017

It is just a proposal. More polish on the way.

@shenli
Copy link
Copy Markdown
Member

shenli commented Apr 21, 2017

@zhexuany Nice work!
@Wenting0905 PTAL

@zhexuany
Copy link
Copy Markdown
Contributor Author

@shenli More work will be done tonight. The goal of this commit is make contribution more newbie friendly.

Comment thread CONTRIBUTING.md Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TiDB is a community-driven open source project and welcomes any contributor.

Comment thread CONTRIBUTING.md Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may be different from other projects that you've been involved in.

Comment thread CONTRIBUTING.md Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before pulling your first request to TiDB, you have to sign the CLA. It is noted that PingCAP only accepts the individual contributor license agreement, which means if your organization is the copyright holder, you can not contribute to this project.

Comment thread CONTRIBUTING.md Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enviorment --> Environment

Comment thread CONTRIBUTING.md Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first step is to fork TiDB to your own repository. After forking TiDB,

@zhexuany zhexuany force-pushed the rewrite_contribution_guid branch 3 times, most recently from 9c516ef to 79f1d3f Compare April 21, 2017 17:05
@zhexuany
Copy link
Copy Markdown
Contributor Author

@Wenting0905 PTAL

Comment thread CONTRIBUTING.md Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TiDB is a community-driven open source project and we welcome any contributor.

Comment thread CONTRIBUTING.md Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

different from many other projects that you've been involved in.

Comment thread CONTRIBUTING.md Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

find a requirement that this doc does not include or meet?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using capture here is identical as using meet

Copy link
Copy Markdown

@Wenting0905 Wenting0905 Apr 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so why not use the more common one?

Comment thread CONTRIBUTING.md Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does this sentence mean?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a requirement for this doc. Sometime, contribution.md may be outdated. Under this circumstance, submitting a new issue about this doc is right practice.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i find it hard to understand the 2nd point

Comment thread CONTRIBUTING.md Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this expression seems strange

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a Metaphor. pre flight check is needed whenever you take a plane. Submit a PR is like flight. Pre checking is necessary to ensure PR in a good quality.

Copy link
Copy Markdown

@Wenting0905 Wenting0905 Apr 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, i know the pre-flight check but i didn't get the point of using a metaphor in a documentation. i hope contributors will understand such a metaphor.

Copy link
Copy Markdown
Contributor Author

@zhexuany zhexuany Apr 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, metaphor is used a lot quite in tech doc. I have read plenty of docs. Metaphor is the easiest way to convey knowledge to audience.

Comment thread CONTRIBUTING.md Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

redundant space.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DONe

Comment thread CONTRIBUTING.md Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small PRs are easy to review while large ones are difficult.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separating for emphasizing.

Comment thread CONTRIBUTING.md Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

your reviewers

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DONE

Comment thread CONTRIBUTING.md Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

redundant space....

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DONE

Comment thread CONTRIBUTING.md Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cannot understand what this sentence means.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well. It is usually related with gofmt.

@zhexuany zhexuany force-pushed the rewrite_contribution_guid branch from 8dd35be to 32cb6e5 Compare April 24, 2017 02:40
@zhexuany
Copy link
Copy Markdown
Contributor Author

@Wenting0905 PTAL

@zhexuany
Copy link
Copy Markdown
Contributor Author

@shenli @coocood PTAL. Most advice from @Wenting0905 is token. It should be ready to merge. But more advice are welcome.

@zhexuany zhexuany force-pushed the rewrite_contribution_guid branch 2 times, most recently from c69d1c6 to 00df4b9 Compare April 25, 2017 17:18
Comment thread CONTRIBUTING.md Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before you move on, please make sure what your issue and/or pull request is, a simple bug fix or an architecture change.

Comment thread CONTRIBUTING.md Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixing bug --> Bug fixing

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug fixes is better.

Comment thread CONTRIBUTING.md Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performance improvement is always difficult to review, even for those experienced software engineers.

Comment thread CONTRIBUTING.md Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. delete the redundant "that"
  2. support --> supports

Comment thread CONTRIBUTING.md Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these --> this
"This" may be better if you want to refer to the event ("not to suggest a performance bug") instead of "examples"

Comment thread CONTRIBUTING.md Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does "it" refer to?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

upstream/master

Comment thread CONTRIBUTING.md Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for making --> to make

Comment thread CONTRIBUTING.md Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for making --> to make

Comment thread CONTRIBUTING.md Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chekc --> Check

Comment thread CONTRIBUTING.md Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some more what?

@zhexuany zhexuany force-pushed the rewrite_contribution_guid branch from 00df4b9 to 44aa6b8 Compare April 26, 2017 07:20
@coocood
Copy link
Copy Markdown
Member

coocood commented Apr 26, 2017

LGTM

@coocood coocood added the status/LGT1 Indicates that a PR has LGTM 1. label Apr 26, 2017
@zhexuany
Copy link
Copy Markdown
Contributor Author

@Wenting0905 @shenli PTAL.

Copy link
Copy Markdown
Member

@shenli shenli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
Good Job!

@shenli shenli merged commit 883ea49 into pingcap:master Apr 27, 2017
@zhexuany zhexuany deleted the rewrite_contribution_guid branch April 29, 2017 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status/LGT1 Indicates that a PR has LGTM 1.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants