Page MenuHomePhabricator

Better boundaries/improved separation within the code
Open, NormalPublic

Description

Some of the possible action items I can see us working on during the WMDE-QWERTY-X-Mas-Sprint-2018-12-18:

  • Pretty much all new Message() in the code should be replaced with $this->msg(). The problem with new Message() is that it relies on global state (the $wgUsers settings as well as the wikis configuration). It is expensive and hard to properly test because of this.
  • While the test coverage is superb (>80%, see https://doc.wikimedia.org/cover-extensions/TwoColConflict/), TwoColConflictHooks is uncovered. Instead of coming up with an arbitrary unit test, we might think of this code as being covered by the browser tests, and mark the methods we think are sufficiently covered via @codeCoverageIgnore.
  • Why is LineBasedUnifiedDiffFormatter a subclass of DiffFormatter? As far as I can see it does not need to. Removing the dependency would free us from certain constraints and unpredictable side-effects.
  • The big switch-case in LineBasedUnifiedDiffFormatter::format needs some love to become easier to read. Note how two cases call private $this->addLines/deleteLines, but one does not. An approach I personally like to follow is to inline such trivial methods first, which allows me to play around with entirely new ways to restructure and possibly split the code.
  • LineBasedUnifiedDiffFormatter::format gets a \Diff objects and converts it to an intermediate array format, which is then consumed by HtmlSplitConflictView::getHtml, which also has a big, hard to read switch-case. The intermediate format contains a lot of duplicate information, some already in HTML form. It might be worth rethinking this intermediate format, reducing the amount of duplication, and striving for a more clear separation between:
    • … code that knows what a \Diff object is, and how it is structured.
    • … code that knows what we want to show on our conflict resolution screen, and the intermediate representation we use for it.
    • … code that creates HTML.

Please extend as needed.

Event Timeline

Restricted Application added a project: TCB-Team. · View Herald TranscriptDec 18 2018, 12:51 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
thiemowmde renamed this task from Improve seperation of code to Better boundaries/improved separation within the code.Dec 18 2018, 3:19 PM
thiemowmde triaged this task as Normal priority.
thiemowmde added a project: Technical-Debt.
thiemowmde updated the task description. (Show Details)
thiemowmde added subscribers: jkroll, WMDE-Fisch, Andrew-WMDE.

Change 480922 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/TwoColConflict@master] Remove not needed subclassing in LineBasedUnifiedDiffFormatter

https://gerrit.wikimedia.org/r/480922

Change 480922 merged by jenkins-bot:
[mediawiki/extensions/TwoColConflict@master] Remove not needed subclassing in LineBasedUnifiedDiffFormatter

https://gerrit.wikimedia.org/r/480922

Change 480963 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/TwoColConflict@master] Replace all new Message() constructors with $this->msg()

https://gerrit.wikimedia.org/r/480963

Change 480980 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/TwoColConflict@master] Rewrite LineBasedUnifiedDiffFormatter switch-case for readability

https://gerrit.wikimedia.org/r/480980

Change 480988 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/TwoColConflict@master] Mark string containing escaped HTML as such via their var names

https://gerrit.wikimedia.org/r/480988

Change 480963 merged by jenkins-bot:
[mediawiki/extensions/TwoColConflict@master] Replace all new Message() constructors with $this->msg()

https://gerrit.wikimedia.org/r/480963

Change 480988 merged by jenkins-bot:
[mediawiki/extensions/TwoColConflict@master] Mark string containing escaped HTML as such via their var names

https://gerrit.wikimedia.org/r/480988

Change 482300 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/TwoColConflict@master] Avoid some &-references in LineBasedUnifiedDiffFormatter

https://gerrit.wikimedia.org/r/482300

Change 480980 merged by jenkins-bot:
[mediawiki/extensions/TwoColConflict@master] Rewrite LineBasedUnifiedDiffFormatter switch-case for readability

https://gerrit.wikimedia.org/r/480980

Change 482300 merged by jenkins-bot:
[mediawiki/extensions/TwoColConflict@master] Avoid some &-references in LineBasedUnifiedDiffFormatter

https://gerrit.wikimedia.org/r/482300

thiemowmde updated the task description. (Show Details)