Skip to content
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.

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

Avoid linkifying trailing question mark #50

Merged
merged 8 commits into from
Nov 11, 2024

Conversation

karlhorky
Copy link
Contributor

@karlhorky karlhorky commented Nov 10, 2024

@karlhorky
Copy link
Contributor Author

karlhorky commented Nov 10, 2024

Oh, looks like snapshot needs to be updated:

linkifyUrlsToHtml: supports CJK URLs

  test.js:148

   1[47](https://github.com/sindresorhus/linkify-urls/actions/runs/11767099536/job/32775410309?pr=50#step:5:48):     t.snapshot(linkify('https://github.com/scarf005/hangul-test/wiki/한…
   1[48](https://github.com/sindresorhus/linkify-urls/actions/runs/11767099536/job/32775410309?pr=50#step:5:49):     t.snapshot(linkify('https://www.例.jp?'));                          
   1[49](https://github.com/sindresorhus/linkify-urls/actions/runs/11767099536/job/32775410309?pr=50#step:5:50):   });                                                                  

  Did not match snapshot

  Difference (- actual, + expected):

  - '<a href="https://www.例.jp">https://www.例.jp</a>?'
  + '<a href="https://www.例.jp?">https://www.例.jp?</a>'

  › file://test.js:148:5



  linkifyUrlsToDom: supports CJK URLs

  test.js:148

   147:     t.snapshot(linkify('[https://github.com/scarf005/hangul-test/wiki/한…](https://github.com/scarf005/hangul-test/wiki/%ED%95%9C%E2%80%A6)
   148:     t.snapshot(linkify('[https://www.例.jp?](https://www.xn--fsq.jp/?)'));                          
   149:   });                                                                  

  Did not match snapshot

  Difference (- actual, + expected):

  - 'DocumentFragment: <a href="[https://www.例.jp](https://www.xn--fsq.jp/)">[https://www.例.jp](https://www.xn--fsq.jp/)</a>?'
  + 'DocumentFragment: <a href="[https://www.例.jp?](https://www.xn--fsq.jp/?)">[https://www.例.jp?](https://www.xn--fsq.jp/?)</a>'

  › file://test.js:148:5

  ─

  2 tests failed

I guess the snapshot update will need to be done locally, since the AVA snapshot format is binary and I can't edit by hand in the GitHub PR

Copy link
Collaborator

@fregante fregante left a comment

Choose a reason for hiding this comment

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

The snapshots can be updated via npx ava -u I think. One is needed for this PR

index.js Outdated Show resolved Hide resolved
index.js Outdated
let trailingDot = '';
if (href.endsWith('.')) {
// The regex URL mistakenly includes punctuation (a period or question mark) at the end of the URL
let punctuation = '';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's just use a regex to extract it here, like:

const punctuation = trailingPunctuationRegex.exec(href) ?? '';
if (punctuation) {
  href = href.slice(0, -1);
}

Copy link
Contributor Author

@karlhorky karlhorky Nov 11, 2024

Choose a reason for hiding this comment

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

Done in 669b814 and 509b9e6

@karlhorky
Copy link
Contributor Author

@fregante Made the suggested change and updated the snapshot

index.js Outdated
let trailingDot = '';
if (href.endsWith('.')) {
// The URL regex mistakenly includes punctuation (a period or question mark) at the end of the URL
const punctuation = /[.?]$/.exec(href) ?? '';
Copy link
Contributor Author

@karlhorky karlhorky Nov 11, 2024

Choose a reason for hiding this comment

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

@fregante I guess it needs to access the first array element with optional chaining (suggestion already committed)

Suggested change
const punctuation = /[.?]$/.exec(href) ?? '';
const punctuation = /[.?]$/.exec(href)?.[0] ?? '';

Repl:

/[.?]$/.exec('asdf.')
> ['.', index: 4, input: 'asdf.', groups: undefined]
/[.?]$/.exec('asdf.').toString()
> '.'

Copy link
Contributor Author

@karlhorky karlhorky Nov 11, 2024

Choose a reason for hiding this comment

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

Oh TIL actually not necessary!

(because of the .toString() method on the .exec() result)

Repl:

'asdf' + /[.?]$/.exec('asdf.')
> 'asdf.'
/[.?]$/.exec('asdf.').toString()
> '.'

I think it's probably still more expressive with the ?.[0]

But if preferred, we can rely on the .toString() "magic" and I can revert this change

Copy link
Collaborator

@fregante fregante left a comment

Choose a reason for hiding this comment

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

Works for me as is. Let's wait for Sindre

@sindresorhus sindresorhus merged commit a90ea64 into sindresorhus:main Nov 11, 2024
3 checks passed
@karlhorky
Copy link
Contributor Author

Thanks for the review and merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants