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

Adding CONTRIBUTING_GUIDELINES.md #864

Merged
merged 7 commits into from
Jun 12, 2018

Conversation

PikBot
Copy link
Contributor

@PikBot PikBot commented Jun 9, 2018

Signed-off-by: Prakriti prakritibansal98@gmail.com

Which problem is this PR solving?

Short description of the changes

  • Adding general contribution guidelines.

Signed-off-by: Prakriti <prakritibansal98@gmail.com>
@codecov
Copy link

codecov bot commented Jun 9, 2018

Codecov Report

Merging #864 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #864   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files         121    121           
  Lines        5997   5997           
=====================================
  Hits         5997   5997

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 10252fe...996aa6d. Read the comment docs.

* Use the imperative mood in the subject line
* Wrap the body at 72 characters
* Use the body to explain _what_ and _why_ instead of _how_
* Uses the active voice (vs. [passive voice](https://www.grammarly.com/blog/a-scary-easy-way-to-help-you-find-passive-voice/)) to make it clear when the user has to perform an action and when actions happen automatically.
Copy link
Member

Choose a reason for hiding this comment

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

this seems unnecessary. Was this copied from the documentation repo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it seemed to contain all of the required content for general guidelines.

* Use the body to explain _what_ and _why_ instead of _how_
* Uses the active voice (vs. [passive voice](https://www.grammarly.com/blog/a-scary-easy-way-to-help-you-find-passive-voice/)) to make it clear when the user has to perform an action and when actions happen automatically.
* Has been spellchecked and uses proper [grammar](https://www.grammarly.com/).
* Has been written in plain language and avoids jargon. Remember, the people reading the documentation know much less about the project than you do.
Copy link
Member

Choose a reason for hiding this comment

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

these 2 lines - same, not a general guideline. We should use CONTRIBUTING from the main repo as the starting point.

* Has been written in plain language and avoids jargon. Remember, the people reading the documentation know much less about the project than you do.
* Each commit must be signed by the author ([see below](#sign-your-work)).

Once the PR is approved and merged, [readthedocs][project] will automatically rebuild the docs.
Copy link
Member

Choose a reason for hiding this comment

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

reference to readthedocs not applicable

of the [Apache License](LICENSE).

If you are adding a new file it should have a header like below. The easiest
way to add such header is to run `make fmt`.
Copy link
Member

Choose a reason for hiding this comment

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

make fmt won't work in every repo

// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
Copy link
Member

Choose a reason for hiding this comment

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

depending on the language of the repo, the comment may start with #


```
git config --add alias.amend "commit -s --amend"
git config --add alias.c "commit -s"
Copy link
Member

Choose a reason for hiding this comment

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

this never worked for me, maybe we can delete it

git config --add alias.c "commit -s"
```

[project]: https://readthedocs.org/projects/jaeger/
Copy link
Member

Choose a reason for hiding this comment

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

not relevant

Signed-off-by: Prakriti <prakritibansal98@gmail.com>
changes ahead of time will make the contribution process smooth for everyone.

Once we've discussed your changes and you've made your updates, then open your PR. Your
pull request is most likely to be accepted if it:
Copy link
Member

Choose a reason for hiding this comment

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

English doesn't work in these bullets

## Making A Change

*Before making any significant changes, please open an
issue.* Discussing your proposed
Copy link
Member

Choose a reason for hiding this comment

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

I think we should elaborate on what we're looking for in new issues (essentially the questions the new-issue-template is asking)

issue.* Discussing your proposed
changes ahead of time will make the contribution process smooth for everyone.

Once we've discussed your changes and you've made your updates, then open your PR. Your
Copy link
Member

Choose a reason for hiding this comment

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

similarly, we should include some guidelines on what should be in the PR description (similar to the template).

Signed-off-by: Prakriti <prakritibansal98@gmail.com>
Signed-off-by: Prakriti <prakritibansal98@gmail.com>
Signed-off-by: Prakriti <prakritibansal98@gmail.com>

- Fixing minor English errors
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

minor correction, lgtm otherwise

* Proposal - what do you suggest to solve the problem or improve the existing situation?
* Any open questions to address

Discussing your proposed changes ahead of time will make the contribution process smooth for everyone. Once we've discussed your changes and you've made your updates, then open your PR. Each PR must contain:
Copy link
Member

Choose a reason for hiding this comment

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

  • s/Once we've discussed your changes and you've made your updates, then open your PR./Once the approach is agreed upon, make your changes and open a pull request (PR)./
  • s/Each PR must contain/Each PR should describe:/

* Any open questions to address

Discussing your proposed changes ahead of time will make the contribution process smooth for everyone. Once we've discussed your changes and you've made your updates, then open your PR. Each PR must contain:
* Which problem is this PR solving?
Copy link
Member

Choose a reason for hiding this comment

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

  • Which problem it is solving. Normally it should be simply a reference to the corresponding issue, e.g. Resolves #123.
  • What changes are made to achieve that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I remove the "short description of the changes" point?

Copy link
Member

Choose a reason for hiding this comment

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

I meant to replace the bullet points with my version.

## Making A Change

*Before making any significant changes, please open an
issue.* Each issue must address the following:
Copy link
Member

Choose a reason for hiding this comment

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

s/Each issue must address the following/Each issue should describe the following:/

* Which problem is this PR solving?
* Short description of the changes

Your pull request is most likely to be accepted if each commit:
Copy link
Member

Choose a reason for hiding this comment

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

"each commit" in bold

* Short description of the changes

Your pull request is most likely to be accepted if each commit:
* Has a [good commit message](https://chris.beams.io/posts/git-commit/):
Copy link
Member

Choose a reason for hiding this comment

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

s/:/. In summary:/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A little unsure about where this change is to be added? After "each commit, in summary:" ?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I was using regexp syntax, "replace : with ...". So

Has a [good commit message](https://chris.beams.io/posts/git-commit/):]. In summary:

By contributing your code, you agree to license your contribution under the terms
of the [Apache License](LICENSE).

If you are adding a new file it should have a header like below. Depending on the language of the repository, the may start with a '#'.
Copy link
Member

Choose a reason for hiding this comment

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

Replace

Depending on the language of the repository, the may start with a '#'./

with

In some languages, e.g. Python, you may need to change the comments to start with #. The easiest way is to copy the header from one of the existing source files and make sure the year is current and the copyright says "The Jaeger Authors".

Signed-off-by: Prakriti <prakritibansal98@gmail.com>
@yurishkuro yurishkuro merged commit 5633f3d into jaegertracing:master Jun 12, 2018
@yurishkuro
Copy link
Member

thanks!

mabn pushed a commit to mabn/jaeger that referenced this pull request Jun 13, 2018
* master:
  Add back quotes
  Adding CONTRIBUTING_GUIDELINES.md (jaegertracing#864)
  Close span writer in standalone (jaegertracing#863)
  Fix Process IP problem/inconsistency (jaegertracing#821) (jaegertracing#865)
  Use NewSpanID()/NewTraceID() instead of type-specific intializations (jaegertracing#861)
  Do not create duplicate child-of reference from parentSpanId (jaegertracing#860)
  Switch to codecov (jaegertracing#857)
  Log configuration options for memory storage (jaegertracing#852)
  Add jpkrohling to maintainers / code owners (jaegertracing#851)
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.

2 participants