Skip to content

Clarify docs on how to start contributing #1056

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

Merged
merged 12 commits into from
Mar 21, 2021
Merged

Clarify docs on how to start contributing #1056

merged 12 commits into from
Mar 21, 2021

Conversation

seisman
Copy link
Member

@seisman seisman commented Mar 14, 2021

Description of proposed changes

Improve the contributing guide to documentation:

  • Mention the .py files in the examples directory
  • Guide users to create a new branch before submitting a PR
  • Mention the "View deployment" button to preview changes
  • Remove a few unnecessary blank lines
  • Remove some trailing whitespaces.
  • Add two subsection headings in the "Documentation" section

Reminders

  • Run make format and make check to make sure the code follows the style guide.
  • Add tests for new features or tests that would have caught the bug that you're fixing.
  • Add new public functions/methods/classes to doc/api/index.rst.
  • Write detailed docstrings for all functions/methods.
  • If adding new functionality, add an example to docstrings or tutorials.

Slash Commands

You can write slash commands (/command) in the first line of a comment to perform
specific operations. Supported slash commands are:

  • /format: automatically format and lint the code
  • /test-gmt-dev: run full tests on the latest GMT development version

CONTRIBUTING.md Outdated
* Click on the "Commit changes" button to open a
[pull request (see below)](#pull-requests).
* Choose "Create a new branch for this commit and start a pull request." and
click on the "Propose changes" button to open a [pull request (see below)](#pull-requests).
Copy link
Member Author

Choose a reason for hiding this comment

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

Users should not commit directly to their master branch. Instead, they should commit to a new branch and submit a PR.

Copy link
Member

Choose a reason for hiding this comment

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

Should we recommend pygmt python-contributors to make branches on the GenericMappingTools/pygmt repo rather than their own fork?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean "python-maintainers"? I don't think "python-contributors" have write permission to the pygmt repo.

Copy link
Member

Choose a reason for hiding this comment

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

They have triage permissions. Doesn't that allow branching?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@weiji14 weiji14 Mar 14, 2021

Choose a reason for hiding this comment

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

Huh, should we bump up the python-contributors team to have "write" permissions then? It's a bit annoying having to add a new git remote to check out a new contributor's code locally, and Leo mentioned before at #415 (comment) that "write" privileges are overrated anyway (i.e. pretty standard for an open source repo).

And just to clarify, the "master" branch is a protected branch so it's not like they will be able to write to that branch accidentally without going through a pull request, passing all checks and getting an approval from a maintainer.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we do, we should add instructions on adding it to your local git repo; for better or for worse GitHub seems to encourage making branches on your own fork.

Copy link
Member Author

Choose a reason for hiding this comment

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

Better to have more discussions at GenericMappingTools/gmt#4732 before making changes.

Copy link
Member

Choose a reason for hiding this comment

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

If we do, we should add instructions on adding it to your local git repo; for better or for worse GitHub seems to encourage making branches on your own fork.

I think it's because it's easier for new Github users to do stuff on their own fork. The instructions can be a bit long though, so would be good if we can just link to a good tutorial instead.

Better to have more discussions at GenericMappingTools/gmt#4732 before making changes.

Ok, I think we can have a 'python'/'pygmt'-only restructure, and it's up to the rest of the GMT ecosystem to decide whether to make the change too. Will make a quick comment on that thread.

Copy link
Member Author

Choose a reason for hiding this comment

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

for better or for worse GitHub seems to encourage making branches on your own fork.

I have a fork in my own account, but the branches are created in the pygmt repo, not in my own fork. Is it because I have admin permission?

This is what I see:

image

@seisman seisman added documentation Improvements or additions to documentation skip-changelog Skip adding Pull Request to changelog labels Mar 14, 2021
@seisman seisman added this to the 0.3.1 milestone Mar 14, 2021
@seisman seisman marked this pull request as ready for review March 14, 2021 06:18
Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

Please change the title to be a bit more specific. E.g. "Clarify docs on how to start contributing".

Plus I would prefer to leave this open for a few more days, so that people have time to leave their feedback, especially the new contributors :)

CONTRIBUTING.md Outdated
* Click on the "Commit changes" button to open a
[pull request (see below)](#pull-requests).
* Choose "Create a new branch for this commit and start a pull request." and
click on the "Propose changes" button to open a [pull request (see below)](#pull-requests).
Copy link
Member

Choose a reason for hiding this comment

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

If we do, we should add instructions on adding it to your local git repo; for better or for worse GitHub seems to encourage making branches on your own fork.

I think it's because it's easier for new Github users to do stuff on their own fork. The instructions can be a bit long though, so would be good if we can just link to a good tutorial instead.

Better to have more discussions at GenericMappingTools/gmt#4732 before making changes.

Ok, I think we can have a 'python'/'pygmt'-only restructure, and it's up to the rest of the GMT ecosystem to decide whether to make the change too. Will make a quick comment on that thread.

@weiji14 weiji14 modified the milestones: 0.3.1, 0.4.0 Mar 14, 2021
@seisman seisman changed the title Improve the contributing guide to documentation Clarify docs on how to start contributing Mar 14, 2021
Copy link
Member

@maxrjones maxrjones left a comment

Choose a reason for hiding this comment

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

A few other things that I would consider improvements:


I think it would be nice to add the subsection headers Building the documentation and Cross-referencing with Sphinx to Documentation


If you get stuck at any point you can create an issue on GitHub (look for the Issues tab in the repository) or contact us at one of the other channels mentioned below.

to

If you get stuck at any point you can create an [issue](https://github.com/GenericMappingTools/pygmt/issues) on GitHub or contact us at one of the other channels [mentioned below](#how-can-i-talk-to-you).


* Make a tutorial or example of how to do something.

to

* Make a tutorial or gallery example of how to do something.


They are executed automatically by sphinx-gallery when the documentation is built

to

They are executed automatically by sphinx-gallery when the [documentation is built](#documentation)

@seisman
Copy link
Member Author

seisman commented Mar 14, 2021

@meghanrjones's suggestions in #1056 (review) are addressed in 9e8ecc1 and 16d05a9.

@seisman
Copy link
Member Author

seisman commented Mar 19, 2021

@meghanrjones @weiji14 @willschlitzer @core-man Please see if you have further comments.

Should we recommend pygmt python-contributors to make branches on the GenericMappingTools/pygmt repo rather than their own fork?

After we set up the team permissions, we can add it to the guide and maybe also document the permissions for each team.

Copy link
Member

@maxrjones maxrjones left a comment

Choose a reason for hiding this comment

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

Looks good!

@core-man
Copy link
Member

Looks good~

Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

Just spotted one URL link to update, otherwise all good :D

@seisman seisman merged commit 13ba2ab into master Mar 21, 2021
@seisman seisman deleted the contributing branch March 21, 2021 15:57
sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
Co-authored-by: Meghan Jones <[email protected]>
Co-authored-by: Wei Ji <[email protected]>
Co-authored-by: Will Schlitzer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation skip-changelog Skip adding Pull Request to changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants