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

Update contribution guides #2624

Merged
merged 11 commits into from
Oct 25, 2023

Conversation

DenisGorbachev
Copy link
Contributor

@DenisGorbachev DenisGorbachev commented Oct 5, 2023

Summary

This PR follows the discussion in "Merge contribution guides?".
This PR's submission was approved by @semorrison.

Currently, there are two guides: CONTRIBUTING.md and doc/contributions.md.

It would be better to merge the guides into a single CONTRIBUTING.md, and link to that file from PULL_REQUEST_TEMPLATE.md.

Notes

I tried to preserve the original wording, but I've shortened some paragraphs & removed ambiguous sentences. Please correct if wrong.

For review, it may be easier to read the final files instead of the diffs.

Questions

  • What should we write in "Feature requests"? (I added my proposal directly to the file)
  • Should we keep "Asking questions" section?
    • In the "Asking questions" section: should we keep the following sentence: "A lot of people report feeling that they are "wasting expert time", but nobody on the team feels this way"? (I propose to keep since it encourages people to reach out)
  • In "Making commits" section, there is a sentence: "Check that your changes do not introduce performance regressions". How exactly should the contributors check for performance regressions?

@DenisGorbachev DenisGorbachev marked this pull request as ready for review October 5, 2023 07:26
@semorrison
Copy link
Collaborator

Too many arbitrary changes that would need discussion.

Perhaps you could try again with the absolute minimal changes required to merge the files? Otherwise we will get to it: even the report about the duplication is very helpful.

@semorrison semorrison closed this Oct 5, 2023
@github-actions github-actions bot added the toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN label Oct 5, 2023
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Oct 5, 2023
@DenisGorbachev
Copy link
Contributor Author

@semorrison I'd love to make minimal changes, but the sentences are interrelated - can't change one without changing the other.

I've fixed some points that you've mentioned and replied to others.

Sorry if I did something wrong - please let me know if it needs more improvements.

@semorrison
Copy link
Collaborator

The basic problem is that doc/contributions.md is the new version that should take priority, and was recently written by Leo. By merging it into the other document is has essentially been lost.

I think the only viable path to combining these documents is:

  • doc/contributions.md stays exactly as is, except moved to CONTRIBUTING.md
  • anything currently in CONTRIBUTING.md which is genuinely additional to anything mentioned in docs/contributions.md could perhaps just be added at the end.

But really anything that is much beyond deleting CONTRIBUTING.md and replacing it with docs/contributions.md unmodifed is going to require too much editorial input to be viable via PR.

@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the builds-mathlib CI has verified that Mathlib builds against this PR label Oct 5, 2023
@leanprover-community-mathlib4-bot
Copy link
Collaborator

leanprover-community-mathlib4-bot commented Oct 5, 2023

@DenisGorbachev
Copy link
Contributor Author

Updated https://github.com/DenisGorbachev/lean4/blob/contribution-guidelines/CONTRIBUTING.md:

  • Fully reused doc/contributions.md
  • Added "Helpful links" section
  • Replaced:
    • Old: In the past, we accepted most pull requests. This practice produced hard to maintain code, performance problems, and bugs. In order to improve the quality and maintainability of our codebase, we've established the following guidelines for external contributions.
    • New: Thank you for your interest in contributing to Lean! There are many ways to contribute and we appreciate all of them.

Reason for replacement: since CONTRIBUTING.md should help people who want to contribute, it's better to start with a welcome message.


Is it correct?

If yes - should I open a new PR or would you like to reopen the current one?

@semorrison
Copy link
Collaborator

Please revert the change from the "replaced:" bullet point. That's a significant editorial change!

@semorrison semorrison reopened this Oct 9, 2023
@semorrison semorrison added the awaiting-author Waiting for PR author to address issues label Oct 9, 2023
@DenisGorbachev
Copy link
Contributor Author

Done

leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Oct 9, 2023
@DenisGorbachev
Copy link
Contributor Author

awaiting-review

(commenting to change the label)

@github-actions github-actions bot added awaiting-review Waiting for someone to review the PR and removed awaiting-author Waiting for PR author to address issues labels Oct 11, 2023
@DenisGorbachev
Copy link
Contributor Author

@semorrison Is there anything else that should be done on this PR?

@semorrison
Copy link
Collaborator

In principle I'm happy now, but the problem is that there is a slide deck for the community meeting that uses the old URL, so I'm very hesitant to merge now. :-)

@semorrison
Copy link
Collaborator

Sorry, on further reflection the right solution is to leave docs/contributions.md in place (it becomes part of the online manual, there are links to it, etc. etc.), and instead to just delete the contents of CONTRIBUTING.md and replace it with "Please see docs/contribuctions.md."

@DenisGorbachev
Copy link
Contributor Author

DenisGorbachev commented Oct 13, 2023

Understood :)

Actually, https://lean-lang.org/lean4/doc/contributions.html returns 404. This is because doc/contributions.md is never mentioned in doc/SUMMARY.md, so it is never rendered by mdBook. Also, in the whole lean4 repo, there are two references to doc/contributions.md: in README.md and in PULL_REQUEST_TEMPLATE.md (both references are fixed in the PR).

Meanwhile, CONTRIBUTING.md is linked to by GitHub on every new PR page (that's just how GitHub works).

Let me know if you'd still like to move the text back to doc/contributions.md or keep it in CONTRIBUTING.md.

@DenisGorbachev
Copy link
Contributor Author

Also, is it OK to provide additional info (like I've done in the previous comment), or would it be better to just implement your suggestions immediately?

@semorrison
Copy link
Collaborator

Thanks for your patience on this one, @DenisGorbachev! Just checking what we want to do here.

@DenisGorbachev
Copy link
Contributor Author

@semorrison Is there any way I can help regarding this PR?

@DenisGorbachev
Copy link
Contributor Author

@semorrison Would you like me to do anything else on this PR?

@semorrison semorrison merged commit d126c09 into leanprover:master Oct 25, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review Waiting for someone to review the PR builds-mathlib CI has verified that Mathlib builds against this PR toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants