Skip to content

[k1LoW/deck#448] fix link text including underscore is partially missing #449

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 5 commits into from
Sep 14, 2025
Merged

[k1LoW/deck#448] fix link text including underscore is partially missing #449

merged 5 commits into from
Sep 14, 2025
+123 −10

Conversation

takaidohigasi
Copy link
Contributor

@takaidohigasi takaidohigasi commented Sep 12, 2025 •
edited
Loading

WHAT

SSIA

WHY

link only handled the first fragment (underscore is one of example)
#448

Note

golden test

https://github.com/k1LoW/deck/blob/0e34436b4952af625c8b104f83573e5299897fb2/testdata/link.md

@takaidohigasi
Copy link
Contributor Author

Error: The action homebrew/actions/setup-homebrew@master is not allowed in k1LoW/deck because all actions must be pinned to a full-length commit SHA.

@Songmu
Copy link
Collaborator

Songmu commented Sep 12, 2025

Your edit is probably correct. Could you please reopen it?

ref. https://spec.commonmark.org/0.31.2/#link-reference-definitions

@Songmu Songmu reopened this Sep 12, 2025
@Songmu
Copy link
Collaborator

Songmu commented Sep 12, 2025

I imagine you've strictly enabled hash pinning, but since the homebrew/actions/setup-homebrew tag doesn't exist, the workflow is broken. Could you please check this? @k1LoW

@k1LoW
Copy link
Owner

k1LoW commented Sep 12, 2025

Sorry! I've disabled it!

@takaidohigasi
Copy link
Contributor Author

thanks, I will check failed test

@takaidohigasi
Copy link
Contributor Author

takaidohigasi commented Sep 12, 2025 •
edited
Loading 

--- FAIL: TestApplyMarkdown (0.00s)
    --- PASS: TestApplyMarkdown/testdata/code.md (15.07s)
    --- PASS: TestApplyMarkdown/testdata/slide.md (20.51s)
    --- PASS: TestApplyMarkdown/testdata/tables.md (25.75s)
    --- PASS: TestApplyMarkdown/testdata/defaults.md (14.93s)
    --- PASS: TestApplyMarkdown/testdata/images.md (33.91s)
    --- PASS: TestApplyMarkdown/testdata/blockquote.md (11.86s)
    --- PASS: TestApplyMarkdown/testdata/codeblock.md (22.62s)
    --- PASS: TestApplyMarkdown/testdata/emoji.md (13.28s)
    --- PASS: TestApplyMarkdown/testdata/bold_and_italic.md (11.48s)
    --- FAIL: TestApplyMarkdown/testdata/paragraphs.md (24.08s)
    --- PASS: TestApplyMarkdown/testdata/breaks_default.md (11.56s)
    --- PASS: TestApplyMarkdown/testdata/list_simple.md (8.23s)
    --- PASS: TestApplyMarkdown/testdata/paragraph_and_list.md (5.45s)
    --- PASS: TestApplyMarkdown/testdata/list_and_paragraph.md (6.75s)
    --- PASS: TestApplyMarkdown/testdata/empty_link.md (6.10s)
    --- FAIL: TestApplyMarkdown/testdata/breaks_enabled.md (24.62s)
    --- PASS: TestApplyMarkdown/testdata/nested_list.md (12.80s)
    --- PASS: TestApplyMarkdown/testdata/br.md (11.66s)
    --- PASS: TestApplyMarkdown/testdata/empty_list.md (4.80s)
    --- PASS: TestApplyMarkdown/testdata/style.md (8.75s)
    --- PASS: TestApplyMarkdown/testdata/cap.md (9.03s)
    --- FAIL: TestApplyMarkdown/testdata/single_list.md (25.72s)

@Songmu
Copy link
Collaborator

Songmu commented Sep 12, 2025

It's recommended to include test cases in the golden tests for clarity.

Songmu
Songmu reviewed Sep 12, 2025
View reviewed changes
md/md.go Outdated
// Concatenate all children values for links
var linkText string
for _, child := range children {
linkText += child.Value
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here, rather than simply concatenating the text, we need to process each child element by converting it into a fragment and adding it, similar to how we handle emphasis. This is to preserve the styles within the link text.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, I think so too, and will fix it.

@takaidohigasi takaidohigasi marked this pull request as ready for review September 12, 2025 22:50
@takaidohigasi
fix: preserve formatting in link text with underscores
a42bfb0 
- Changed implementation to create separate fragments for each text node
- This preserves formatting (bold, italic, code) within links
- Added comprehensive tests for links with underscores and formatting
@takaidohigasi takaidohigasi force-pushed the gh-448-fix-link-text-including-underscore branch from 9367384 to a42bfb0 Compare September 12, 2025 23:06
@takaidohigasi
fix: update remaining example.com URLs in golden file
12d5bfd 
- Replaced all example.com URLs with github.com/k1LoW/deck in link.md.golden
- Ensures consistency across all test data
Songmu
Songmu requested changes Sep 13, 2025
View reviewed changes
Copy link
Collaborator

@Songmu Songmu left a comment

Choose a reason for hiding this comment

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

Great!
No issues with the code changes.

Please adjust the test code.

  • md/link_test.go duplicates the golden test, so let's remove it
  • Should we change [**bold_start** normal_middle **bold_end**] to [**bold_start** normal_middle __bold_end__]?
    • This time, the issue stemmed from the underscore being a special character in Markdown that triggers token splitting. We want to ensure our test cases cover this scenario as well.

@Songmu Songmu added the bug Something isn't working label Sep 13, 2025
@takaidohigasi
test: address review feedback
c56c1d1 
- Remove md/link_test.go as it duplicates golden test functionality
- Change **bold_end** to __bold_end__ in test case to better test underscore handling
  (double underscores also create bold formatting and test token splitting)
@takaidohigasi
Copy link
Contributor Author

Should we change [bold_start normal_middle bold_end] to [bold_start normal_middle bold_end]?

thanks, good test case. I fixed for it.

@Songmu Songmu merged commit e954043 into k1LoW:main Sep 14, 2025
1 check passed
@github-actions github-actions bot mentioned this pull request Sep 14, 2025
Merged
@Songmu
Copy link
Collaborator

Songmu commented Sep 14, 2025

Great work.
I appreciate you handling the complex fixes in the end. Thank you.

@Songmu Songmu mentioned this pull request Sep 14, 2025
Merged
@BrewTestBot BrewTestBot mentioned this pull request Sep 15, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Songmu Songmu Songmu approved these changes

Assignees
No one assigned
Labels
bug Something isn't working integration-test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@takaidohigasi @Songmu @k1LoW