Skip to content

Comments

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

Merged
Songmu merged 5 commits intok1LoW:mainfrom
takaidohigasi:gh-448-fix-link-text-including-underscore
Sep 14, 2025
Merged

[k1LoW/deck#448] fix link text including underscore is partially missing#449
Songmu merged 5 commits intok1LoW:mainfrom
takaidohigasi:gh-448-fix-link-text-including-underscore

Conversation

@takaidohigasi
Copy link
Contributor

@takaidohigasi takaidohigasi commented Sep 12, 2025 •
edited
Loading

@takaidohigasi
Copy link
Contributor Author

takaidohigasi commented Sep 12, 2025

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

takaidohigasi commented Sep 12, 2025

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

@Songmu Songmu Sep 12, 2025

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

@takaidohigasi takaidohigasi Sep 12, 2025

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

takaidohigasi commented Sep 14, 2025

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