Page MenuHomePhabricator

Figure out if new empty line treatment really improves things
Closed, ResolvedPublic8 Story Points


NB this is timeboxed to 5 hours

Figure out if we really create an improvement in diffs by introducing the new empty line treatment. To do so, please follow the process we established for changes in moved paragraphs, which means:

  • generate random article samples
  • check if they were improved / worsened/ unchanged (imagining we had implemented the whole feature, where line numbers are correct and emptly lines are seperate visual entities)
  • document the results

Event Timeline

Lea_WMDE created this task.Feb 6 2018, 4:49 PM
Lea_WMDE triaged this task as Normal priority.
Lea_WMDE removed WMDE-Fisch as the assignee of this task.Feb 7 2018, 3:42 PM
Lea_WMDE updated the task description. (Show Details)
Lea_WMDE added a subscriber: WMDE-Fisch.
WMDE-Fisch claimed this task.
Lea_WMDE updated the task description. (Show Details)Feb 20 2018, 4:17 PM
Tobi_WMDE_SW set the point value for this task to 8.Feb 20 2018, 4:28 PM

During my investigation on that topic I got some regression on the following diffs. These can be used as examples to look into way to improve the issue.

Removed empty line and small text involved:

Move of a line above an empty line:

Adding an empty line and text:

WMDE-Fisch removed WMDE-Fisch as the assignee of this task.Feb 23 2018, 12:24 PM
WMDE-Fisch added subscribers: Andrew-WMDE, Addshore.

Since we decided it might be better, if someone else looks at a way to implement this. I unassign myself here @jkroll @Addshore @Andrew-WMDE. The examples above can be used as edge cases that need to be taken care off.

Lea_WMDE closed this task as Resolved.
Lea_WMDE claimed this task.

This has been resolved differently by looking at paragraphs that have only been moved one line. For more details see the patch posted below.

jkroll added a comment.Mar 7 2018, 6:58 PM

unfortunately, the diffs generated using this special-case code turned about to be incorrect so i had to revert this. looking at the list of random test diffs, these cases turn out to be rare though.