Skip to content

Conversation

loriab
Copy link
Member

@loriab loriab commented Nov 23, 2021

Description

This is something I was working on in the spring to find docs problems earlier (currently we'll maybe notice if they break in master) and to remove docs building responsibilities from PR submitters. It looks to still be in working order.

The main contentions are likely to be how to phrase the auto-posts so that PR submitters aren't freaked out and inspired to jumble their git histories beyond repair. Below is a screenshot of what it posts to a PR. I can already see things I'd change. Opening this PR to start discussion on how the auto-post should read.

Screen Shot 2021-11-23 at 2 49 00 PM

Status

  • Ready for review
  • Ready for merge

@loriab loriab added this to the Psi4 1.6 milestone Nov 23, 2021
@loriab
Copy link
Member Author

loriab commented Nov 25, 2021

latest wording:
Screen Shot 2021-11-24 at 8 52 55 PM

@PeterKraus
Copy link
Contributor

I'd prefer to keep the blurb simple, and if at all possible, separate tests from docs. Why not go with something like:

@loriab : 
Your PR is modifying Psi4's documentation. To make your life easier, we've built 
the updated docs for you. You can preview your changes online at [netlify/...](), 
or locally by downloading [this zip archive]().

Please incorporate any further changes by adding commits to this PR.

The samples issue is more complex. I'd avoid trying to teach folks how to use git in an automated blurb.

@loriab:
Your PR is modifying Psi4's test suite. To keep the `samples` suite in sync with
`tests`, we have created an automated commit [checksum]() to your PR branch.

Please review, merge, or otherwise address the changes proposed in the automated commit 
into your PR before merging the PR into Psi4's `master`. Feel free to ask for help in 
the PR if you have any questions.

@JonathonMisiewicz
Copy link
Contributor

I don't understand what Peter means by "separate tests from docs". I suspect the word "tests" should be "samples," and I have no concrete idea as to how this PR combines them. My best guess is have "a build of the documentation target" not be what re-generates samples.

I strongly disagree with removing git command line instructions. They will work in 99% of cases, and not following these instructions could cause merge conflicts that will take more effort from core devs to shepherd new devs through.

For the sample regeneration section: Give examples of why changes might not be okay. Move the bullet point explaining the point of the auto-commit up a level.

For the documentation section: I don't understand the point of the first bullet point. Explain why an edit to this branch might be needed. Explain the difference between deploy-preview and the sphinxman-html download. Make clear that editing the PR branch is also needed for the sphinxman-html download.

@hokru
Copy link
Member

hokru commented Nov 29, 2021

Lori's message is for the experienced developer while Peter's is for the beginner.
I'd also aim more for the beginner, but include the git commands for the reasons Jonathon mentioned.

@loriab
Copy link
Member Author

loriab commented Nov 29, 2021

  • I suspect that what Peter means by separate tests and docs is to issue two comments to the PR? I'm worried ppl will read the last and overlook the former.
  • Sounds like I need to make clearer that the only choice for the samples/ commit is now or later. There are no valid reasons to reject, but it you're going to be editing tests a lot, you might not want a dozen automated commits or to have your remote tampered with while local is in a vulnerable state.
  • I'm with keeping the git commands as I suspect many ppl are nervous about --rebase and --force and need that reassurance. I do wish I could convey that you're done with local, no need for either path.

Thanks for the comments! I'll do another round of commit-message tweaking. Eventually we can add linters by the same mechanism, so I think it pays to get clear communication and git sync habits established here.

@loriab loriab mentioned this pull request Jan 11, 2022
@loriab loriab removed this from the Psi4 1.6 milestone May 11, 2022
@loriab loriab added this to the Psi4 1.7 milestone May 11, 2022
@loriab loriab mentioned this pull request Jun 14, 2022
5 tasks
@loriab loriab modified the milestones: Psi4 1.7, Psi4 1.8 Nov 28, 2022
@loriab loriab modified the milestones: Psi4 1.8, Psi4 1.9 Apr 24, 2023
@loriab
Copy link
Member Author

loriab commented Nov 13, 2023

The does-your-PR-break-the-docs and archive-a-tarball-of-your-PRs-docs-for-you-to-check-offline aspects of this PR are long fulfilled. The let-netlify-build-you-a-website-preview-of-your-PRs-docs and have-bot-push-updates-of-samples-to-your-PR aspects are complicated by security and by people having to understand why they can't simple (not force) push to their own PR branches. I don't think these latter aspects are worth the hassle.

@loriab loriab closed this Nov 13, 2023
@loriab loriab removed this from the Psi4 1.9 milestone Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants