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

Rework the development documentation #5526

Merged
merged 20 commits into from
Jul 23, 2018

Conversation

pradyunsg
Copy link
Member

@pradyunsg pradyunsg commented Jun 23, 2018

The idea here is to make the development processes a bit more approachable to potential contributors by clearly documenting how to work on the project and grouping other information into separate pages.

This PR does so by break up the existing development documentation into 4 parts, with the following structure/changes:

  • 🆕 An inviting introduction that links to various support channels.
    • idea + links taken from warehouse's docs
  • Getting Started
  • Release processes
    • Release Cadence, Deprecation Policy and how to do releases
  • Contributing
    • What's expected from a PR
      • reworded to not talk about how to make a PR
    • NEWS entries
      • 🆕 Broken up into 3 sections: why, how and "which extension".
    • 🆕 How to update (i.e. rebase) a branch/PR
      • derived from warehouse's docs

@brainwane If you could take a look at this and provide some feedback, that'd be awesome. :)

@dstufft The bots would need to be updated for this. This is basically blocked until we figure out a suitable way to change them to link to the new location. IIUC, the URLs would change due to this PR and break any existing links.

As per my understanding, we'd need to update these links:

  • BrownTruck: can link to "Updating your branch"
  • pypa-bot: NEWS fragment information
  • PR template: NEWS fragment information (this is basically in the repo, so it's cool)

I would love to feedback on this from existing contributors and obviously @pypa/pip-committers.

@pradyunsg pradyunsg added type: docs Documentation related state: blocked Can not be done until something else is done type: maintenance Related to Development and Maintenance Processes labels Jun 23, 2018
@pradyunsg pradyunsg added this to the Internal Cleansing milestone Jun 23, 2018
@pradyunsg pradyunsg self-assigned this Jun 23, 2018
@pradyunsg pradyunsg requested review from dstufft, brainwane and a team June 23, 2018 11:21
docs/development/getting-started.rst Outdated Show resolved Hide resolved

The built documentation can be found in the ``docs/build`` folder.

.. _`open an issue`: https://github.com/pypa/pip/issues/new?title=Truouble+with+pip+development+environment
Copy link
Member Author

Choose a reason for hiding this comment

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

Whee. Typo!

sections below will help you get started with development, testing, and
documentation.

Please contribute issues, submit bug reports, file feature requests, and help
Copy link
Member Author

Choose a reason for hiding this comment

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

This section, as a whole, could possibly be worded better.

Copy link
Member

Choose a reason for hiding this comment

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

One thing I think we should mention here is that we support Python 2.7 and 3, and we support Windows and Unix. Contributions must consider all of those cases. While a contribution from someone who only works on Python 2.7 and Unix is appreciated, it's not going to progress unless someone puts in the work to consider Python 3 and Windows, and I think we need to set contributors' expectations here. More generally, contributions need to go beyond "works for me". As a team, we don't have the manpower to take incomplete contributions and complete them ourselves (although we will happily help the contributor to do so with advice and suggestions).

Copy link
Member Author

@pradyunsg pradyunsg Jun 23, 2018

Choose a reason for hiding this comment

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

That is probably something that should be clarified in the contributing page, in the "Submitting Pull Requests" section.

Copy link
Member Author

Choose a reason for hiding this comment

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

On the point of setting contributors' expectations early, totally agree. I think this should mention that pip is a volunteer maintained project.

We could also link from the PR template to the "Submitting Pull Requests" section (while also talking about NEWS fragments).

Copy link
Member Author

Choose a reason for hiding this comment

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

Coming back around to this, I don't even think this paragraph is even needed here. We don't need to list various ways to interact with the project on GitHub in the "development" documentation.

-------------

pip uses the :pypi:`pytest` test framework, :pypi:`mock` and :pypi:`pretend`
for testing. These are automatically installed by tox for running the tests.
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we also say "and various pytest plugins" here?

Copy link
Member

Choose a reason for hiding this comment

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

IMO no, we don't need to complicate things by adding that much detail.

@pradyunsg pradyunsg added the skip news Does not need a NEWS file entry (eg: trivial changes) label Jun 23, 2018
@pradyunsg
Copy link
Member Author

@pypa/pip-committers I'm on the fence as to whether this should get an entry in the NEWS file. On the one hand, it doesn't concern most users. On the other hand, we have a dedicated processes section where this could go and get some visibility.

-------------

pip uses the :pypi:`pytest` test framework, :pypi:`mock` and :pypi:`pretend`
for testing. These are automatically installed by tox for running the tests.
Copy link
Member

Choose a reason for hiding this comment

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

IMO no, we don't need to complicate things by adding that much detail.

docs/development/getting-started.rst Outdated Show resolved Hide resolved
docs/development/getting-started.rst Show resolved Hide resolved
sections below will help you get started with development, testing, and
documentation.

Please contribute issues, submit bug reports, file feature requests, and help
Copy link
Member

Choose a reason for hiding this comment

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

One thing I think we should mention here is that we support Python 2.7 and 3, and we support Windows and Unix. Contributions must consider all of those cases. While a contribution from someone who only works on Python 2.7 and Unix is appreciated, it's not going to progress unless someone puts in the work to consider Python 3 and Windows, and I think we need to set contributors' expectations here. More generally, contributions need to go beyond "works for me". As a team, we don't have the manpower to take incomplete contributions and complete them ourselves (although we will happily help the contributor to do so with advice and suggestions).

docs/development/release-process.rst Outdated Show resolved Hide resolved
docs/development/release-process.rst Outdated Show resolved Hide resolved
docs/development/release-process.rst Outdated Show resolved Hide resolved
docs/development/release-process.rst Outdated Show resolved Hide resolved
docs/development/release-process.rst Outdated Show resolved Hide resolved
docs/development/release-process.rst Outdated Show resolved Hide resolved
@pradyunsg pradyunsg removed the skip news Does not need a NEWS file entry (eg: trivial changes) label Jun 23, 2018
@pradyunsg
Copy link
Member Author

(removed the label so it's harder to accidentally merge)

@pradyunsg pradyunsg removed the request for review from brainwane July 15, 2018 10:48
@pradyunsg
Copy link
Member Author

I'm sorry, I don't have time to look at this.

No issues. Thanks for pinging others who might be able to provide feedback here.

Any and all feedback is welcome. :)

@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Jul 17, 2018
@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Jul 20, 2018
@theacodes
Copy link
Member

This looks great! I'd say go ahead and merge this and iterate if needed.

@pradyunsg
Copy link
Member Author

Thanks for taking a look @theacodes! ^>^


I was thinking about removing the "blocking" that the linking to "Writing a NEWS entry" involves here. I guess just adding a temporary? section/note saying that has been moved to a different location should suffice for now. :)

@pradyunsg pradyunsg added skip news Does not need a NEWS file entry (eg: trivial changes) and removed state: blocked Can not be done until something else is done labels Jul 23, 2018
@pradyunsg
Copy link
Member Author

I'll go ahead and merge this since this is a (big!) improvement over status quo and as @theacodes suggests, we can iterate on it. :)

@pradyunsg pradyunsg merged commit 1e09062 into pypa:master Jul 23, 2018
@pradyunsg pradyunsg deleted the docs/improve-dev-docs branch July 23, 2018 19:38
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 24, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation skip news Does not need a NEWS file entry (eg: trivial changes) type: docs Documentation related type: maintenance Related to Development and Maintenance Processes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't run tests with py.test
8 participants