Skip to content

Fix multi line symbol link source range issue #151

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 2 commits into from
Oct 11, 2023
Merged

Fix multi line symbol link source range issue #151

merged 2 commits into from
Oct 11, 2023

Conversation

Kyle-Ye
Copy link
Contributor

@Kyle-Ye Kyle-Ye commented Oct 5, 2023

Bug/issue #, if applicable:

Close #142

Summary

Fix the unexpected source range issue for multi line symbol link

For detail see #142 (comment)

2 question to be discussed:

  1. Should we add CMARK_OPT_SOURCEPOS by default the same as CMARK_OPT_TABLE_SPANS?
  2. Should we add a new ParseOptions case so that client of Markdown can choose to opt-in or opt-out this option?

Dependencies

None

Testing

Add testMultilineSymbolLink test case

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

@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Oct 5, 2023

@swift-ci please test

@Kyle-Ye Kyle-Ye mentioned this pull request Oct 5, 2023
Closed
2 tasks
@d-ronnqvist d-ronnqvist self-requested a review October 5, 2023 17:05
@d-ronnqvist
Copy link
Contributor

d-ronnqvist commented Oct 5, 2023 •
edited
Loading

  1. Should we add CMARK_OPT_SOURCEPOS by default the same as CMARK_OPT_TABLE_SPANS?
  2. Should we add a new ParseOptions case so that client of Markdown can choose to opt-in or opt-out this option?
  1. From what I understand the performance impact of CMARK_OPT_SOURCEPOS should be very small so we I feel that we could enable it by default.
  2. Regardless if we enable it by default or not, adding a ParseOption so that callers can opt in or out would be good.

@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Oct 6, 2023 •
edited
Loading

Added the ParseOption.

Another thing that confuses me is that CMARK_OPT_SOURCEPOS seems to have other side effects in addition to solving this problem.

image

Also according to the doc of CMARK_OPT_SOURCEPOS - "Include a data-sourcepos attribute on all block elements.", it looks like the sourcepos output on xml is actually its main propose and the impact on Inline's source range is its side effects IMO. 🧐

@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Oct 6, 2023

@swift-ci please test

@Kyle-Ye Kyle-Ye merged commit 4312077 into swiftlang:main Oct 11, 2023
@Kyle-Ye Kyle-Ye deleted the bugfix/backtick_codevoice branch October 11, 2023 09:46
Kyle-Ye added a commit to Kyle-Ye/swift-markdown that referenced this pull request Oct 17, 2023
@Kyle-Ye
Fix multi line symbol link source range issue (swiftlang#151)
8b24b70 
* Fix multi line symbol link source range issue

* Add disableSourcePosOpts ParseOptions
@Kyle-Ye Kyle-Ye mentioned this pull request Oct 17, 2023
Merged
@Kyle-Ye Kyle-Ye linked an issue Oct 25, 2023 that may be closed by this pull request
convert and preview commands crash on certain Markdown pages swiftlang/swift-docc#678
Closed
2 tasks
d-ronnqvist pushed a commit that referenced this pull request Nov 14, 2023
@Kyle-Ye
Fix multi line symbol link source range issue (#151) (#156)
d91b618 
* Fix multi line symbol link source range issue

* Add disableSourcePosOpts ParseOptions
cgrindel-self-hosted-renovate bot referenced this pull request in cgrindel/rules_swift_package_manager Sep 20, 2024
@cgrindel-self-hosted-renovate
chore(deps): update dependency apple/swift-markdown to v0.5.0 (#1244)
e552bdb 
This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
|
[apple/swift-markdown](https://redirect.github.com/apple/swift-markdown)
| minor | `0.4.0` -> `0.5.0` |

---

### Release Notes

<details>
<summary>apple/swift-markdown (apple/swift-markdown)</summary>

###
[`v0.5.0`](https://redirect.github.com/swiftlang/swift-markdown/releases/tag/0.5.0):
Swift-Markdown 0.5.0

[Compare
Source](https://redirect.github.com/apple/swift-markdown/compare/0.4.0...0.5.0)

This release is based on the code that shipped into Swift 6.0.

#### What's Changed

- Add link title support for commonmark by
[@&#8203;Kyle-Ye](https://redirect.github.com/Kyle-Ye) in
[https://github.com/swiftlang/swift-markdown/pull/140](https://redirect.github.com/swiftlang/swift-markdown/pull/140)
- Remove dependency on argument-parser by
[@&#8203;ethan-kusters](https://redirect.github.com/ethan-kusters) in
[https://github.com/swiftlang/swift-markdown/pull/149](https://redirect.github.com/swiftlang/swift-markdown/pull/149)
- Fix utf8 decode by
[@&#8203;zunda-pixel](https://redirect.github.com/zunda-pixel) in
[https://github.com/swiftlang/swift-markdown/pull/145](https://redirect.github.com/swiftlang/swift-markdown/pull/145)
- Fix multi line symbol link source range issue by
[@&#8203;Kyle-Ye](https://redirect.github.com/Kyle-Ye) in
[https://github.com/swiftlang/swift-markdown/pull/151](https://redirect.github.com/swiftlang/swift-markdown/pull/151)
- Fix multiline directive without content parsing range issue by
[@&#8203;Kyle-Ye](https://redirect.github.com/Kyle-Ye) in
[https://github.com/swiftlang/swift-markdown/pull/154](https://redirect.github.com/swiftlang/swift-markdown/pull/154)
- build: add a CMake based build by
[@&#8203;compnerd](https://redirect.github.com/compnerd) in
[https://github.com/swiftlang/swift-markdown/pull/141](https://redirect.github.com/swiftlang/swift-markdown/pull/141)
- Add 2024 as an accepted year number by
[@&#8203;Kyle-Ye](https://redirect.github.com/Kyle-Ye) in
[https://github.com/swiftlang/swift-markdown/pull/160](https://redirect.github.com/swiftlang/swift-markdown/pull/160)
- Fix Directive argument name and value ranges on non-first line by
[@&#8203;Kyle-Ye](https://redirect.github.com/Kyle-Ye) in
[https://github.com/swiftlang/swift-markdown/pull/79](https://redirect.github.com/swiftlang/swift-markdown/pull/79)
- Add support for Doxygen discussion/note tags by
[@&#8203;j-f1](https://redirect.github.com/j-f1) in
[https://github.com/swiftlang/swift-markdown/pull/159](https://redirect.github.com/swiftlang/swift-markdown/pull/159)
- Add support for formatting the new Doxygen types using MarkupFormatter
by [@&#8203;j-f1](https://redirect.github.com/j-f1) in
[https://github.com/swiftlang/swift-markdown/pull/163](https://redirect.github.com/swiftlang/swift-markdown/pull/163)
- Support Windows ARM64 builds by
[@&#8203;hjyamauchi](https://redirect.github.com/hjyamauchi) in
[https://github.com/swiftlang/swift-markdown/pull/164](https://redirect.github.com/swiftlang/swift-markdown/pull/164)
- build: silence warning about CMakeLists.txt from SPM by
[@&#8203;compnerd](https://redirect.github.com/compnerd) in
[https://github.com/swiftlang/swift-markdown/pull/167](https://redirect.github.com/swiftlang/swift-markdown/pull/167)

#### New Contributors

- [@&#8203;zunda-pixel](https://redirect.github.com/zunda-pixel) made
their first contribution in
[https://github.com/swiftlang/swift-markdown/pull/145](https://redirect.github.com/swiftlang/swift-markdown/pull/145)
- [@&#8203;j-f1](https://redirect.github.com/j-f1) made their first
contribution in
[https://github.com/swiftlang/swift-markdown/pull/159](https://redirect.github.com/swiftlang/swift-markdown/pull/159)
- [@&#8203;hjyamauchi](https://redirect.github.com/hjyamauchi) made
their first contribution in
[https://github.com/swiftlang/swift-markdown/pull/164](https://redirect.github.com/swiftlang/swift-markdown/pull/164)

**Full Changelog**:
swiftlang/swift-markdown@0.4.0...0.5.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the
rebase/retry checkbox.

👻 **Immortal**: This PR will be recreated if closed unmerged. Get
[config
help](https://redirect.github.com/renovatebot/renovate/discussions) if
that's undesired.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Renovate
Bot](https://redirect.github.com/renovatebot/renovate).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC44OS4xIiwidXBkYXRlZEluVmVyIjoiMzguODkuMSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOltdfQ==-->

Co-authored-by: cgrindel-self-hosted-renovate[bot] <139595543+cgrindel-self-hosted-renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@d-ronnqvist d-ronnqvist d-ronnqvist approved these changes

@QuietMisdreavus QuietMisdreavus Awaiting requested review from QuietMisdreavus

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

Successfully merging this pull request may close these issues.

Unexpected symbol link source range convert and preview commands crash on certain Markdown pages
2 participants
@Kyle-Ye @d-ronnqvist