17
\$\begingroup\$

On this site, we have a well-known policy on editing code in questions: Don't. After all, that code is up for review, in all its glorious ugliness and buginess. But clearly, such reasoning cannot apply equally to code in answers.

I was therefore surprised to find that a nice edit on one of my answers had been rejected for reasons that would only make sense if this were code in a question. The edit was attempted twice unsuccessfully (review 1, review 2), and rejected for the following reasons:

  • This edit deviates from the original intent of the post. Even edits that must make drastic changes should strive to preserve the goals of the post's owner.

    No, it did not change any intent, but merely fixed the faulty expression of my (the answerer's) intent.

  • Fixing the author's code for them removes a potential point for reviewers.

    No, this is an answer, not a question. Fixing simple mistakes should be all right.

  • Please don't edit OP's code. Comment on the post instead. Code blocks are for OP to fix on this site.

    Citation needed! Why wouldn't it be OK to make an edit to the code in an answer?

(In the meanwhile, I invited the editor to re-submit the edit, and have already approved it. Ergo, this is a discussion on general site policy using that edit only as a post-mortem example.)

Since the last reason was given by a mod, I trawled through the help center and meta in search of policy guidance relating to answer edits, but there is none. All I could find were discussions on question edits.

  1. What is the site policy on answer edits? Are they held to the same strict standards as question edits, or do we allow a more collaborative style of answering?

  2. Regarding this specific edit, is there a community consensus on whether the edit was reviewed correctly? If so, could you please link to existing codifications of answer editing guidelines? As it stands, I assume all those edit rejection reasons to be unfounded.

In either case, I agree with RubberDuck that “we really should write down our editing guidelines anyway. Otherwise, we'll someday end up with 2k users who don't know any better.

\$\endgroup\$
  • 4
    \$\begingroup\$ Attempting a disproved edit again is a red-flag for me personally. \$\endgroup\$ – Mast Oct 22 '15 at 9:45
  • 4
    \$\begingroup\$ This is relevant largely to the original situation, but I was quick to reject when I saw this was a previously rejected edit that was being re-suggested, which is generally a bad practice. It is better to put as a comment if your edit is rejected once so that someone else can respond and determine if it's a good edit. Ideally the OP of course. \$\endgroup\$ – SuperBiasedMan Oct 22 '15 at 9:46
  • 6
    \$\begingroup\$ Commenting really is much better than editing. I've had erroneous edits inflicted on my answers before. I'm ashamed to admit that I've also inflicted bug-inducing edits on others before learning my lesson. \$\endgroup\$ – 200_success Oct 22 '15 at 10:15
  • 4
    \$\begingroup\$ There also may be a technical difficulty here. I find it rather hard to distinguish questions, answers and tag wikis in the review queue (with tag wikis being the easy one out), so I err on the side of caution and usually apply question standards, so: no code edits \$\endgroup\$ – Vogel612 Oct 22 '15 at 10:20
  • \$\begingroup\$ @Vogel612 That's true, the only indication is the “answered” vs. “asked” light-grey label over the user card in the review UI. But curtailing the community just because of shitty UI would seem short-sighted. I'd rather put the onus on the reviewer to consider the full context – err on the side of viewing the full post. Or skip the review if you aren't sure what you're doing. \$\endgroup\$ – amon Oct 22 '15 at 10:28
11
\$\begingroup\$

The Edit Review queue is notorious for people making mistakes in the process. The problem is almost entirely related to the fact that answer edits are much rarer than question edits.

Code Review has unfortunately got a lot of robo-reviewers in this queue, people who see a code edit - and auto-reject - without first checking whether the code is part of an answer, or a question.

In the past 2 years I have been on the site I don't think I have seen a single "bad" code-edit on an answer in the review queue. In almost every case it is people fixing the answers after they have tried to run the code and the code fails due to typos, copy/paste, or simple logic errors that the answerer made.

I have approved many suggested edits to my own posts that correct my numerous fat-figering (an unfortunate monkey side-effect).

Solution

The solution is that code suggested edits, if they have an edit comment that makes logical/obvious sense, should just be approved. If there's a problem, well, the answerer whose code was edited will get a notification of the edit, and they can easily roll it back if it's wrong.

There is no need to set up a comment/response cycle that @amon has suggested. That's too slow, and requires humans to check things later, and is error prone.

The examples @amon shows are classic. The edit comment is:

Comment: Logic corrected on if loop. The previous logic is incorrect, after one generation all cells become dead because they are not counted properly. Now the cell isn't counted only if i AND j are equal to row_idx and idx. Before, when only one row_idx or ix was equal to i or j the cell was discarted.

To me, that is obvious that the editor has run the code, debugged it, and is has heart that is 2 sizes bigger and is contributing the efforts of his work back to the community. Give them the 2-rep points, make the site better, and approve the edit. It's the right thing to do. (Then, go and find some other good contributions they have on the site, a question, an answer, something, and if it is worthy, upvote it, so that they are closer to having the reputation where a review is no longer needed!).

\$\endgroup\$
  • 5
    \$\begingroup\$ I usually go by the rule: When in doubt, skip is always an option. \$\endgroup\$ – Simon Forsberg Oct 22 '15 at 17:25
  • \$\begingroup\$ can't quite figure out what you mistyped in the first sentence in the last paragraph, in ...and is has heart that is 2 sizes bigger... \$\endgroup\$ – Stop ongoing harm to Monica Oct 22 '15 at 17:25
  • \$\begingroup\$ @janos - youtube.com/watch?v=_eulSbXIjzk \$\endgroup\$ – rolfl Oct 22 '15 at 17:31
7
\$\begingroup\$

Answer edits are great, and improve the answer for everyone! Some kinds of edits are universally good:

  • fixing spelling errors
  • improving formatting
  • improving grammar
  • adding material to back up claims, e.g. links to a styleguide

Other kinds of edits ought to be their own answer:

  • discussing aspects of the code that were not covered in the original answer
  • offering alternative solutions

If the code in an answer contains a mistake, things are more tricky: Maybe that code was part of the question, and the answerer merely didn't spot the bug? Consider adding your own answer if this is a substantial bug. Maybe the answerer was excessively clever, and you misunderstood the code? Ask for clarification in a comment. In either case, clever code is bad, and ought to be simplified.

But in most cases, it's a genuine mistake. To err is human, and aside from a monkey and a rubber duck, we're all just humans here. Unfortunately, making an edit to fix the code can be difficult to validate by the edit reviewers. Therefore:

  1. Comment on the answer and ask for clarification. Maybe that code is intended that way, maybe it's a real mistake.
  2. If the answerer explains that the code works as intended, you've learnt something new.
  3. If the answerer confirms the bug, make an edit. In the edit description, summarize the reasoning for your change and mention that the answerer has approved.
  4. If the answerer doesn't respond after a reasonable time frame, just make the edit – the worst that can happen is that it's rejected. In the edit description, summarize your reasoning for your change and refer to your comment for an in-depth description.
  5. You've made Code Review a better place – congrats!

It would be reasonable to follow this procedure even when you can make edits without having to go through a review. Collaboratively improving the answer is just way nicer than having to roll back a bad edit.

\$\endgroup\$
3
\$\begingroup\$

Please don't edit OP's code. Comment on the post instead. Code blocks are for OP to fix on this site.

I wrote this... referring to question edits policy - I made a mistake, the edit could have been approved. My apologies to @AxeEffect, going by the book it should have gone through.


But...

There's a "but". How many times have you seen comments like this on Stack Exchange?

Shouldn't Foo be Bar instead? - User1234 3 minutes ago

Oh right, nice catch! Edited, thanks! - User9876 42 seconds ago

I think that's how the system works best, simply because it only involves the people that have all the context: suggesting an edit to do it for the author is ok too, but without edit privs, it introduces other humans, and the more humans involved, the higher the risk of human error.

So yes, the edit(s) should have been approved. Oh, well.


So, what are the guidelines for answer edits?

@amon's answer puts them nicely. I'd only add that putting [note: answer edit] in the edit comment when there's code block edits involved, could help reviewers fully grasp the context of the edit, and might help prevent this from happening again in the future - although yes, indeed, it's completely the reviewer's responsibility to make sure they're taking the right decision.

As reviewers, the first thing we need to ask ourselves should be "is this a question or an answer edit?", not "what's the diff like?" or "are there any missed edit opportunities that could improve this edit?".

\$\endgroup\$
  • \$\begingroup\$ How many time have you seen this on Stack Overflow: "Shouldn't Foo be Bar instead? - User1234 3 minutes ago" and nothing else? \$\endgroup\$ – rolfl Oct 22 '15 at 14:59
  • \$\begingroup\$ @rolfl but, we're not Stack Overflow ;-) \$\endgroup\$ – Mathieu Guindon Oct 22 '15 at 15:00
  • \$\begingroup\$ But we are stack exchange..... also note that, after users have enough rep, there's no review queue at all - what's stopping people then? \$\endgroup\$ – rolfl Oct 22 '15 at 15:01
  • 4
    \$\begingroup\$ Users are encouraged to make good edits. You should not make users have to jump through ugly hoops because reviewers are too lazy and rushed to do their jobs properly. This is an anti-solution. The reviewer who rejected those edits should be review-banned, or warned ;-) \$\endgroup\$ – rolfl Oct 22 '15 at 15:02
  • \$\begingroup\$ @rolfl nothing should be stopping them, we want these edits. now how do I review-ban myself? Note that this "anti-solution" answer is saying exactly that: "it's completely the reviewer's responsibility to make sure they're taking the right decision." and "As reviewers, the first thing we need to ask ourselves should be "is this a question or an answer edit?"" - this answer is saying the problem is with the reviewers, not with the answer edits. \$\endgroup\$ – Mathieu Guindon Oct 22 '15 at 15:10

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .