Skip to content

store ordered list start index from cmark #22

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 8 commits into from
Oct 5, 2022
Merged

store ordered list start index from cmark #22

merged 8 commits into from
Oct 5, 2022

Conversation

QuietMisdreavus
Copy link
Contributor

Bug/issue #, if applicable: rdar://73847907

Summary

While cmark-gfm stores the start index for an ordered list, swift-markdown currently discards this information. This PR is part of a set to use this start index when handling ordered lists.

I also added license headers to the markdown in the doc bundle so that bin/check-source can pass.

Dependencies

None

Testing

With the following markdown test file, ensure that the debug tree properly contains the start index of the list.

2. this is a test
3. this is a test
4. this is a test

Steps:

  1. swift run markdown-tool dump-tree test.md
  2. Compare the output to the below sample and ensure that the start index is preserved:
Document
└─ OrderedList start: 2
   ├─ ListItem
   │  └─ Paragraph
   │     └─ Text "this is a test"
   ├─ ListItem
   │  └─ Paragraph
   │     └─ Text "this is a test"
   └─ ListItem
      └─ Paragraph
         └─ Text "this is a test"

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

  • Added tests
  • Ran the ./bin/test script and it succeeded
  • Updated documentation if necessary

This was referenced Dec 8, 2021
Merged
Merged
@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test

franklinsch
franklinsch reviewed Dec 13, 2021
View reviewed changes
franklinsch
franklinsch reviewed Dec 13, 2021
View reviewed changes
Sources/Markdown/Base/RawMarkup.swift Outdated
@@ -24,7 +24,7 @@ enum RawMarkupData: Equatable {
case thematicBreak
case htmlBlock(String)
case listItem(checkbox: Checkbox?)
case orderedList
case orderedList(start: Int?)
Copy link
Contributor

@franklinsch franklinsch Dec 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is an API breaking change, this will require a coordinated change with Swift-DocC, right? Is there a way we could introduce the change without requiring the coordination?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I think this could benefit from a default value (orderedList(start: Int? = nil))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The RawMarkup type isn't used in Swift-DocC, so this won't cause source breakage there. The public accessor to the start index is the new start property on the OrderedList type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this again, this type (RawMarkupData) isn't public API. Other enum cases don't use a default value; that's enforced via the public Markup types which wrap RawMarkup. I don't think it would be useful to include a default value for the list start value here; that comes from cmark.

@nighthawk nighthawk mentioned this pull request Mar 9, 2022
Merged
2 tasks
nighthawk added a commit to maparoni/down-gfm that referenced this pull request Mar 12, 2022
@nighthawk
Switch to swift-markdown (#3)
909b4c6 
Changes:

- Removes embedded `cmark` in favour of [`apple/swift-cmark`](https://github.com/apple/swift-cmark)
- Removes AST code in favour of [`apple/swift-markdown`](https://github.com/apple/swift-markdown)
- Removes DebugVisitor in favour of `document.debugDescription()` from `swift-markdown`
- Support tasklist/checkbox

Later:

- Restore ordered list starts, depends on: swiftlang/swift-markdown#22
- Restore various Down options, depends on: swiftlang/swift-markdown#23
@nighthawk nighthawk mentioned this pull request Mar 12, 2022
Draft
@QuietMisdreavus
Copy link
Contributor Author

I've rebased this onto the latest main (and dropped the commit that added copyrights to the doc markdown files, since that was added separately). I've also updated the doc comment for OrderedList.start to better reflect the default value, and updated the implementation to allow ordered lists to start with zero (since cmark and HTML allow this).

franklinsch
franklinsch reviewed Jul 12, 2022
View reviewed changes
@QuietMisdreavus QuietMisdreavus mentioned this pull request Aug 1, 2022
Closed
This was referenced Sep 15, 2022
Open
Closed
Closed
@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test

@QuietMisdreavus
Copy link
Contributor Author

Had to update a test after reverting the change to the markup formatter.

@swift-ci Please test

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test

@QuietMisdreavus QuietMisdreavus merged commit d491147 into swiftlang:main Oct 5, 2022
@QuietMisdreavus QuietMisdreavus deleted the list-numbering branch October 5, 2022 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@franklinsch franklinsch franklinsch approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@QuietMisdreavus @franklinsch