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

DOC: "Committing your code" in "Contributing to pandas" needs update #48035

Closed
1 task done
marenwestermann opened this issue Aug 11, 2022 · 3 comments · Fixed by #48113
Closed
1 task done

DOC: "Committing your code" in "Contributing to pandas" needs update #48035

marenwestermann opened this issue Aug 11, 2022 · 3 comments · Fixed by #48113
Labels

Comments

@marenwestermann
Copy link
Contributor

Pandas version checks

  • I have checked that the issue still exists on the latest versions of the docs on main here

Location of the documentation

https://pandas.pydata.org/docs/development/contributing.html#committing-your-code

Documentation problem

In the "Committing your code" section of the "Contributing to pandas" documentation there are two problems.

1. It is said:

Please reference the relevant GitHub issues in your commit message using GH1234 or #1234.

However, I don't see that this convention is still in use. It seems to be common to reference the respective issue in the PR description, such as "closes #1234".

2. It is further said:

Either style is fine, but the former is generally preferred:

  • a subject line with < 80 chars.
  • One blank line.
  • Optionally, a commit message body.

Now you can commit your changes in your local repository:

git commit -m

The command git commit -m raises an error that no commit message is given. There are two possibilities here:

  1. Change the command to git commit -m "commit message comes here", which users can do to have a single line commit message. It is possible to have a multi line commit message this way, but this doesn't seem to be used frequently.
  2. Change the command to git commit which opens an editor for the user where they can write a multiline commit message as described above. However, since the project seems to almost always use "squash and merge", I'm not sure how those commit messages are handled.

Suggested fix for documentation

For issue No 1:
Since the convention of including the respective GitHub issue in the commit message doesn't seem to be in use anymore, the documentation for this could be removed.

For issue No 2:
It would be nice to make the documentation of how the commit messages should be written/ structured consistent with the conventions of the core developers.

I'm happy to update the documentation myself after decisions on how to proceed have been made.

@marenwestermann marenwestermann added Docs Needs Triage Issue that has not been reviewed by a pandas team member labels Aug 11, 2022
@marenwestermann
Copy link
Contributor Author

ping @noatamir

@noatamir
Copy link
Member

Thanks for the issue @marenwestermann!

  1. I think your observation is correct, when a PR closes an issue it should state closes #1234, and I believe this is in our PR template as well. I would prefer that the documentation clarifies what you observed, rather than remove the mention of the related issue entirely.

Therefore, I would suggest removing this from the commit section, and clarifying it in the PR section, since it is a part of the PR step. WDYT

  1. I agree about the need to update the command to git commit -m "your commit message goes here". All code blocks which we can copy should be executable.

I am uncertain if there are strong conventions to be clarified. In my experience as a contributor so far, commit messages are easy going. But I'd better let the maintainers chime in on this, as I might be unaware of previous discussions or preferences.

  1. I am uncertain whether this should be added here as well or not. But I noticed there are a few new emerging conventions for commit message prefixes:
  • REGR for regression tests rather than TST.
  • DEPR is used for deprecation issues.
  • WEB for website issues.
  • QST for questions.
  • CI for CI related issues.

@phofl
Copy link
Member

phofl commented Aug 11, 2022

We are using the title of the PR when merging into main, so the actual commit messages on the branch itself don't matter much. Maybe we can clarify this.

@lithomas1 lithomas1 removed the Needs Triage Issue that has not been reviewed by a pandas team member label Aug 18, 2022
@lithomas1 lithomas1 added this to the Contributions Welcome milestone Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants