Page MenuHomePhabricator

Implement basic book referencing
Closed, ResolvedPublic8 Estimate Story Points

Description

I suggest to split the task of implementing the new extends="…" attribute into smaller technical tickets, starting with this one here:

  • In the text, a <ref> with the new attribute is rendered with a sub-number like "1.3".
  • In the <references /> section, the reference is rendered with indentation and sub-numbering.
  • Include parser tests if applicable.
  • Don't deal with any edge cases, where possible. STATUS: accidentally dealt with almost every edge case.
  • Don't touch VisualEditor use cases.

Depends on: T236257

This is the recent proof-of-concept, which visually accomplishes the task here: https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/Cite/+/530399/

Details

Related Gerrit Patches:
mediawiki/extensions/Cite : masterExtract stack and state to a new class
mediawiki/extensions/Cite : masterFix footnote mark after extends numbering glitch
mediawiki/extensions/Cite : masterNumbering bug: Parser test which should fail
mediawiki/extensions/Cite : masterDon't leave unclosed <li> behind
mediawiki/extensions/Cite : master[WIP] Render nested references
mediawiki/extensions/Cite : masterRender nested references
mediawiki/extensions/Cite : masterExtract numbering functions to separate classes
mediawiki/extensions/Cite : masterMerge two more code paths in Cite::referencesFormatEntry()
mediawiki/extensions/Cite : masterBlock de-facto empty <ref> as if it's empty
mediawiki/extensions/Cite : masterCollapse duplicate code in Cite::referencesFormatEntry()
mediawiki/extensions/Cite : masterFix indention and add comments to referencesFormatEntry()
mediawiki/extensions/Cite : masterAvoid intermediate array when rendering a <references> list
mediawiki/extensions/Cite : masterRemove dead code from Cite::referencesFormatEntry()
mediawiki/extensions/Cite : masterReduce duplicate code in Cite::stack()

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 4 2019, 11:16 AM

We're discussing rewriting this ticket without the proposed, intermediate implementation. We would go straight to indented and sub-numbered references.

awight updated the task description. (Show Details)Nov 6 2019, 12:59 PM
awight set the point value for this task to 5.
awight renamed this task from Implement baseline and/or fallback for non-indented sub-references to Implement basic book referencing.Nov 12 2019, 12:55 PM

Indentation could be split out into its own patches, if that makes the implementation easier.

Change 550672 had a related patch set uploaded (by Awight; owner: Awight):
[mediawiki/extensions/Cite@master] [WIP] Extract numbering functions to separate classes

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

Change 551525 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/Cite@master] Remove dead code from Cite::referencesFormatEntry()

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

Change 551530 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/Cite@master] Avoid intermediate array when rendering a <references> list

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

Change 551536 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/Cite@master] Fix indention and add comments to referencesFormatEntry()

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

Change 551539 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/Cite@master] Collapse duplicate code in Cite::referencesFormatEntry()

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

Change 551554 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/Cite@master] [WIP] Render nested references

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

Change 551558 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/Cite@master] Reduce duplicate code in Cite::stack()

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

Change 551525 merged by jenkins-bot:
[mediawiki/extensions/Cite@master] Remove dead code from Cite::referencesFormatEntry()

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

Change 551558 merged by jenkins-bot:
[mediawiki/extensions/Cite@master] Reduce duplicate code in Cite::stack()

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

Change 551530 merged by jenkins-bot:
[mediawiki/extensions/Cite@master] Avoid intermediate array when rendering a <references> list

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

Change 551536 merged by jenkins-bot:
[mediawiki/extensions/Cite@master] Fix indention and add comments to referencesFormatEntry()

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

Change 551539 merged by jenkins-bot:
[mediawiki/extensions/Cite@master] Collapse duplicate code in Cite::referencesFormatEntry()

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

awight updated the task description. (Show Details)Nov 20 2019, 1:07 PM
awight changed the point value for this task from 5 to 7.

Change 552073 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/Cite@master] Block de-facto empty <ref> as if it's empty

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

Change 552073 merged by jenkins-bot:
[mediawiki/extensions/Cite@master] Block de-facto empty <ref> as if it's empty

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

Izno added a subscriber: Izno.Nov 20 2019, 7:38 PM

Change 552073 merged by jenkins-bot:
[mediawiki/extensions/Cite@master] Block de-facto empty <ref> as if it's empty
https://gerrit.wikimedia.org/r/552073

  1. That summary is rather obtuse. Please make it more clear the intent.
  2. You should probably split this into its own task because you are changing rendering behavior here for wikitext that already exists. It's not appropriate to do it as part of this ticket in that regard.
  3. Reminds me of T157418: RFC: Make some aspects of Tidy's whitespace stripping behavior part of wikitext parsing "spec". Maybe one of the sorts on that ticket might have an opinion.
Izno added a comment.Nov 20 2019, 8:06 PM

And probably a 4. The error message displayed is insufficient. Someone looking through the wikitext is going to look for an actually-empty item, not one that is empty save for whitespace.

Thanks a lot for taking care! I really hope we did not forgot something, but we are all humans. Let's see:

  1. Are you talking about the first line in the commit message? Yes, that could have been more clear. Sorry. For reference: The intent was that <ref> with no content behave the same as <ref> with no visible content.
  2. Yes, that would have been better. However, we consider this not really a change in behavior, but a bugfix. Before, is was possible to have <ref> with no visible content, but no way to find them.
  3. I'm not sure what you are referring to. T157418 appears to be an entirely different, way larger can of worms.
  4. I checked, and it seems the two relevant error messages don't talk about the <ref> being "empty", but "must have content" and "no text". Both do sound good to me. Did I missed something? Which error message do you think is a problem?
Izno added a comment.Nov 21 2019, 2:38 PM
  1. Yes, that would have been better. However, we consider this not really a change in behavior, but a bugfix. Before, is was possible to have <ref> with no visible content, but no way to find them.

It's not a bug fix if no-one asked for it (decent rule of thumb for Wikimedians). If you're changing how stuff is rendered in wikitext especially. See also below comment.

  1. I'm not sure what you are referring to. T157418 appears to be an entirely different, way larger can of worms.

And it's already done. The reason I'm mentioning the task is to ask whether we should apply the same notion of trimming to random XML-like wikitext tags ad hoc. The resolution in that task for example was that wikitext HTML tags do not trim whitespace (according to the HTML 5 construction rules), and only some wikitext does have whitespace trimmed. Do we want an ad hoc change here for all reference tags to trim whitespace? Start getting into questions of whether other tags should trim whitespace. Is a wikitext XML-like tag more like HTML 5 (and should it follow the HTML 5 rules) or is it more like wikitext and should it be allowed to strip whitespace? Not strip whitespace? All such tags? Ad hoc?

  1. Are you talking about the first line in the commit message? Yes, that could have been more clear. Sorry. For reference: The intent was that <ref> with no content behave the same as <ref> with no visible content.
  2. I checked, and it seems the two relevant error messages don't talk about the <ref> being "empty", but "must have content" and "no text". Both do sound good to me. Did I missed something? Which error message do you think is a problem?

No visible content is still content and needs to indicated as such. With a warning that says "no content" or even "no text" (why are those different warnings?), the user will look for ></ref> rather than something like >\w</ref> (pseudo-regex) in the wikitext.

I would expect the commit summary to be "treat whitespace-only ref tags like no-content ref tags" or "trim whitespace in ref tags", since that's what you're doing.

As I said,

It's not appropriate to do it as part of this ticket.

Change 552296 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/Cite@master] Merge two more code paths in Cite::referencesFormatEntry()

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

Change 552296 merged by jenkins-bot:
[mediawiki/extensions/Cite@master] Merge two more code paths in Cite::referencesFormatEntry()

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

Change 552546 had a related patch set uploaded (by Awight; owner: Awight):
[mediawiki/extensions/Cite@master] Extract stack and state to a new class

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

Change 552546 merged by jenkins-bot:
[mediawiki/extensions/Cite@master] Extract stack and state to a new class

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

thiemowmde changed the point value for this task from 7 to 8.Nov 26 2019, 12:33 PM

Change 550672 abandoned by Awight:
Extract numbering functions to separate classes

Reason:
Looking at Icd933fc98, I don't think these services are relevant to our work at the moment. Abandoning as out of scope.

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

Change 553111 had a related patch set uploaded (by Awight; owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/Cite@master] [WIP] Render nested references (fork)

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

Change 553111 merged by jenkins-bot:
[mediawiki/extensions/Cite@master] Render nested references

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

Change 553759 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/Cite@master] Don't leave unclosed <li> behind

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

Change 551554 abandoned by Awight:
[WIP] Render nested references

Reason:
Merged the alternate fork.

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

Change 553759 merged by jenkins-bot:
[mediawiki/extensions/Cite@master] Don't leave unclosed <li> behind

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

awight added a comment.Dec 9 2019, 8:44 AM

We've hit a lot of numbering issues already, I'll just do this here rather than create a new task. On current master, we have:

<ref name="a">First</ref>
<ref extends="a">Second</ref>
<ref extends="a">Third</ref>
<ref name="d">Fourth</ref>
<hr />
<references />

Rendering as,

Change 555902 had a related patch set uploaded (by Awight; owner: Awight):
[mediawiki/extensions/Cite@master] Numbering bug: Parser test which should fail

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

awight added a comment.Dec 9 2019, 8:59 AM

We've hit a lot of numbering issues already, I'll just do this here rather than create a new task. On current master, we have:

Bisecting shows I broke this in rECIT3f276388bff

Change 555906 had a related patch set uploaded (by Awight; owner: Awight):
[mediawiki/extensions/Cite@master] Fix footnote mark after extends numbering glitch

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

awight updated the task description. (Show Details)Dec 9 2019, 9:28 AM
awight moved this task from Doing to Demo on the WMDE-QWERTY-Sprint-2019-11-20 board.

Change 555902 merged by jenkins-bot:
[mediawiki/extensions/Cite@master] Numbering bug: Parser test which should fail

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

Change 555906 merged by jenkins-bot:
[mediawiki/extensions/Cite@master] Fix footnote mark after extends numbering glitch

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

WMDE-Fisch closed this task as Resolved.Jan 7 2020, 12:59 PM
WMDE-Fisch moved this task from Demo to Done on the WMDE-QWERTY-Sprint-2019-12-11 board.

This ticket was meant as an umbrella for a first version of the feature. I consider it done. Any other things we want to work on in this context should go into their own tickets.