Add a new check for copyediting

[adrn]

This fixes #104.

This adds a new PR check that fails if the label "docs" is set unless the label "copyedited" has been added.

Note that this depends on functionality added to baldrick in OpenAstronomy/baldrick#76, and so shouldn't be merged until that PR is resolved and merged.

[codecov]

Codecov Report

Merging #105 into master will decrease coverage by 7.58%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #105      +/-   ##
==========================================
- Coverage   98.61%   91.02%   -7.59%     
==========================================
  Files           3        4       +1     
  Lines         144      156      +12     
==========================================
  Hits          142      142              
- Misses          2       14      +12

Impacted Files	Coverage Δ
astropy_bot/copyedit_checker.py	0% <0%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd2c34e...14d2cc2. Read the comment docs.

[bsipocz]

Many of the recent Docs PRs don't really need copy editing as they are minor fixes (https://github.com/astropy/astropy/pulls?q=is%3Apr+is%3Aclosed+label%3ADocs). Do we really want to overload @lglattly to sign off on all of them?

[adrn]

We'll leave that up to the maintainer that looks at the PR. (We will add guidelines about this in the developer docs, and via an email to astropy-dev). But basically: if a maintainer adds the "copyedited" label, this indicates that the PR doesn't need copyediting, and she will skip it.

We discussed adding a no-copyediting-needed label, but decided that was overengineering.

[pllim]

Maybe this can be a configurable item in pyproject.toml? If other packages decides to deploy this in their own repo, you don't want lglattly to get spammed.

[adrn]

haha! Right.

[adrn]

Although, it might be a way to get @lglattly some more work ;)

[bsipocz]

We'll leave that up to the maintainer that looks at the PR. (We will add guidelines about this in the developer docs, and via an email to astropy-dev). But basically: if a maintainer adds the "copyedited" label, this indicates that the PR doesn't need copyediting, and she will skip it.

That seems a bit much for the usual few liners. Anyway, I'm comfortable saying something doesn't need copy editing, but not comfortable saying that something has been copy edited.

What happened with the original idea of staying need-copy-editing and when it's done the label gets removed?

[adrn]

@eteq and I discussed and decided that it would be easier to forget and accidentally merge before copyediting was done if it requires a maintainer to manually add the "needs" label. This way, it requires an action to ignore it.

[adrn]

but not comfortable saying that something has been copy edited.

@bsipocz So you would prefer a "no-copyediting-required" label?

[bsipocz]

This way, it requires an action to ignore it.

and that action will be done by the ones who do the labeling most of the time anyways.

I'm still 👎 on this unless a better name comes up for the label. I won't set copyedited labels as I'm not qualified to make that statement for anything I had a look at.

[adrn]

I don't actually think those doing the labeling should have to set this: setting "copyedited" or, if we make it, "no-copyediting-needed" should be set by whoever reviews the PR, IMO.

[bsipocz]

Then who would set it? I might misunderstood something. Isn't the proposal that everything that is labeled by Docs should go through copyediting and if those PRs doesn't also have a copyedited label then their status will be failing?
So we either start merging failing status PRs which would trigger the OCD in some of us, or start adding copyedited labels to PRs that are Docs but doesn't need copy editing as being trivial changes (like most that are Docs).
On the other hand, major new feature PRs may not have the Docs label as they are new features, yet they add significant amount of new narrative docs. Yet this mechanism wouldn't pick those up to be something that also needs to go through copyediting.

[bsipocz]

This a needs-copy-edit seems to be a better approach to me. Yes, it could still happen that PRs accidentally get merged without, but we are usually quite good at adding abundant whats-new-needed etc labels, it's quite unlikely (assuming that the status will fail with such a label).

[adrn]

Then who would set it?

I just explained: the PR reviewer.

I'm not suggesting we merge failing PRs: I'm suggesting that, in order to skip the check, the PR reviewer needs to deliberately indicate that copyediting should be skipped. In my initial proposal, that was by way of setting the existing "copyedited" label. But yes, another option would be to have a "no-copyediting-needed" label.

[bsipocz]

I just explained: the PR reviewer.

who is in many times for those little docs PRs is @pllim or me as we do the labeling anyway and approve trivial stuff like missing backticks, changelog fixes, etc that are all Docs labelled.

[bsipocz]

So yes, please bring it up with a wider contributor pool than just @eteq and ask what others prefer. An opt in needs-copy-edit that follows the logic of whats-need-needed, or more default labeling for each and every docs PR.

[adrn]

Got it: I didn't appreciate that you and @pllim end up reviewing docs PRs in the course of labeling. We're discussing more options with @crawfordsm and @kelle. More soon...

[bsipocz]

Well, both of us are motivated to keep the number of open PRs under control and not to ping subpackage maintainers with trivial stuff when they have bigger fish to fry.

[eteq]

After some discussion, I think the goal here is to make a clear set of tasks for the copyeditor(s) to do. So here's a proposal for what to do:

1. Don't have the copy-editing be a block for merging in any circumstance. (Which may obviate the need for this PR, but it was a way to start the discussion...)
2. Have the copy-editing-needed label be automatic for docs PRs (which may or may not be hard to do with the bot?)
3. For other PRs, it's opt-in by whoever thinks the label should be added
4. It's the copyeditor's job to remove labels when the copy editing has been done.

Longer-term, we would want to set some sort of infrastructure that actually diffs the docs and the copy-editor can drive their work using that rather than the actual PRs/issues. But that requires a non-trivial amount of setup/maintainence so it would be a future project.

Does that 1-4 seem reasonable, @bsipocz ?

[adrn]

Note: under this new proposal, since copyediting isn't a requirement to merge, we would also want to make sure that the copyediting-needed label stays on a PR after it is merged, so @lglattly could in principle review any relevant text after merging. I don't love this, but I also don't want a bottleneck to happen where a bunch of PRs are waiting on copyediting.

[bsipocz]

I still don't mind it being a blocker if it's an opt in system. E.g. we manually add the copy edit needed for PRs that actually need copy editing.

E.g. I'm not sure it's the ones with Docs labelled that actually need the copy edit the most, but the ones like astropy/astropy#8517 and astropy/astropy#8540, yet I don't think we should add the Docs label for these (but in both cases I think it's reasonable to wait with the merge until there is copyediting feedback (which can be a "go ahead, it will get the copyedit later" one).

[pllim]

Have the copy-editing-needed label be automatic for docs PRs (which may or may not be hard to do with the bot?)

In theory, it should be easy for the bot to auto-add "copy-editing-needed" label when "docs" label is present and the former is not added. I am not 100% of the race condition behavior if maintainer manually adds "docs" and then adds "copy-editing-needed" right after, also manually, but hopefully this is rare occasion.

What the bot cannot do is in a situation where the PR is primarily not about docs, but doc is added as part of the code change. We usually don't label "docs" for those, at least not consistently.

Or... if we do #65 first, then maybe this will be even easier. Because in that case, labels are automatically added by the bot, and maintainers curate those afterwards.

[astrofrog]

I'm coming to this conversation late, but based on where this conversation is going, is this PR still needed since copyediting isn't a requirement to merge? Please request a review from me again if you do end up wanting it to be reviewed.

[adrn]

Yep, I guess the conclusion is that this isn't needed - I'll close