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 #875

Merged
merged 3 commits into from
May 24, 2019
Merged

Add contributing guidelines #875

merged 3 commits into from
May 24, 2019

Conversation

jd
Copy link
Contributor

@jd jd commented Apr 4, 2019

No description provided.

@jd jd requested a review from a team as a code owner April 4, 2019 23:12
@jd
Copy link
Contributor Author

jd commented Apr 4, 2019

This is just a draft. Feel free to submit your ideas @majorgreys @brettlangdon.

Copy link
Member

@brettlangdon brettlangdon left a comment

Choose a reason for hiding this comment

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

It'd be nice to have a section explaining that ddtrace.internal should only be used within the ddtrace module namespace, that breaking API changes can happen at any time to that module, etc etc.

We might also want to consider adding a GitHub issue/pull request template. Can include some basic things like "did you enable debug logging", can you give us a pip freeze, etc etc.

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated
chance that the implementation appears doable in several steps. In order to
facilite the review process, we strongly advise to split your feature
implementation in small pull requests (if that is possible) so they contain a
very small number of commits (a single commit per pull request being optimal).
Copy link
Member

Choose a reason for hiding this comment

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

I'm not too concerned with number of commits in a PR, we do squash merging anyways. Keeping emphasis on "small PRs" is 👍 though!

CONTRIBUTING.md Outdated
That ensures that:

1. Each commit passes the test suite.
2. The human review process is easier.
Copy link
Member

Choose a reason for hiding this comment

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

🙇

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was this confusing? I've added a explanation.

CONTRIBUTING.md Outdated

## Python Versions Support

Python versions that are supported are the Python version that are supported by
Copy link
Member

Choose a reason for hiding this comment

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

We should probably actually fully list out all supported versions here.

This way we can be explicit about it being CPython only, and in the off case we need to support a non-community supported version for a customer, then we can potentially list it here too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added CPython, though I'm a bit reluctant to have the list of version supported listed here, 'cause it'll make us need to maintain and update it.
That's what I meant by "supported by the community". e.g. Python 3.4 is EOL.

Maybe it's unclear the way it's written?

CONTRIBUTING.md Outdated
Python versions that are supported are the Python version that are supported by
the community.

## Libraries Support
Copy link
Member

Choose a reason for hiding this comment

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

I like this section, we might want to loosen the wording here a bit. I don't want customers to think we will YOLO just drop support for a given library or version. We want to make sure they are confident in using our library, that using it will "just work".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've enhanced this a bit.

@jd
Copy link
Contributor Author

jd commented Apr 10, 2019

Yeah, I don't have enough experience yet that I feel confident about written an issue template, but if you have a common list of questions you know you have to ask on each issue, feel free to list them somewhere. I can then start another PR :)

@jd jd changed the base branch from 0.24-dev to 0.25-dev April 16, 2019 06:52
@jd
Copy link
Contributor Author

jd commented Apr 30, 2019

Moved to RST, updated workflow

@jd jd changed the base branch from 0.25-dev to master May 9, 2019 13:37
@jd
Copy link
Contributor Author

jd commented May 24, 2019

Any reason we don't merge this @DataDog/apm-python ?

@jd jd merged commit 1b9d3f5 into DataDog:master May 24, 2019
@jd jd deleted the guidelines branch May 24, 2019 12:20
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.

3 participants