Skip to content

More autolinks fixed (For #82)#91

Merged
nobodywasishere merged 15 commits intoicyleaf:masterfrom
ralsina:fix/autolinks
May 9, 2025
Merged

More autolinks fixed (For #82)#91
nobodywasishere merged 15 commits intoicyleaf:masterfrom
ralsina:fix/autolinks

Conversation

@ralsina
Copy link
Copy Markdown
Contributor

@ralsina ralsina commented May 8, 2025

This PR makes most of the tests in spec/fixtures/gfm-extensions.txt:542 pass (see below) and also enables a few more cases which pass if the autolinks extension is enabled.

There are 2 cases that are failing:

First, which I can probably fix but I am just tired:

mmmmailto:scyther@pokemon.com

Should be: "<p>mmmmailto:<a href=\"mailto:scyther@pokemon.com\">scyther@pokemon.com</a></p>\n"

Is: "<p>mmm<a href=\"mailto:scyther@pokemon.com\">mailto:scyther@pokemon.com</a></p>\n"

Second, which is just wrong because that is not a valid domain (domains MUST contain a dot according to the same spec):

**Autolink and http://inlines**

Should be: "<p><strong>Autolink and <a href=\"http://inlines\">http://inlines</a></strong></p>\n"

Is: "<p><strong>Autolink and http://inlines</strong></p>\n"

If it's edited to have a proper domain like inlines.com it works properly.

@trafico-bot trafico-bot Bot added the 🔍 Ready for Review Pull Request is not reviewed yet label May 8, 2025
@ralsina ralsina changed the title More autolinks fixed More autolinks fixed (For #82) May 8, 2025
@nobodywasishere nobodywasishere mentioned this pull request May 8, 2025
@ralsina
Copy link
Copy Markdown
Contributor Author

ralsina commented May 9, 2025

Sigh, that mmmmailto:foo@bar.com test case is so arbitrary 🤣

The spec for the autolinks extension says:

"All such recognized autolinks can only come at the beginning of
a line, after whitespace, or any of the delimiting characters *, _, ~,
and (."

But to match the testcase it has to be detecting foo@bar.com after a :

I can only fix it by doing a lot of special-casing for a stupid corner case which is (I think) just wrong.

@nobodywasishere
Copy link
Copy Markdown
Collaborator

I'd be fine with pulling that out into its own testcase and marking it as pending so the testcase with the rest of them working passes

@nobodywasishere
Copy link
Copy Markdown
Collaborator

nobodywasishere commented May 9, 2025

@ralsina
Copy link
Copy Markdown
Contributor Author

ralsina commented May 9, 2025

mmmmailto:scyther@pokemon.com

mailto:scyther@pokemon.com

aaamailto:scyther@pokemon.com

mmmmailto: scyther@pokemon.com

mailto: scyther@pokemon.com

aaamailto: scyther@pokemon.com

Makes sense, coming up

@ralsina
Copy link
Copy Markdown
Contributor Author

ralsina commented May 9, 2025

Done.

@nobodywasishere nobodywasishere merged commit 246420c into icyleaf:master May 9, 2025
5 checks passed
@trafico-bot trafico-bot Bot added ✨ Merged Pull Request has been merged successfully and removed 🔍 Ready for Review Pull Request is not reviewed yet labels May 9, 2025
@ralsina ralsina deleted the fix/autolinks branch May 9, 2025 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

✨ Merged Pull Request has been merged successfully

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants