Skip to content

fix: recognize IPv4 address literals in email hosts - #122

Closed
spokodev wants to merge 1 commit into
markdown-it:masterfrom
spokodev:fix/ipv4-address-literal-mail-host
Closed

spokodev wants to merge 1 commit into
markdown-it:masterfrom
spokodev:fix/ipv4-address-literal-mail-host

Conversation

@spokodev

@spokodev spokodev commented Sep 2, 2026

Copy link
Copy Markdown

linkify-it already recognizes the IPv6 address-literal email host form (user@[IPv6:::1], added in 6.0.0), but not the IPv4 counterpart. Per RFC 5321 §4.1.3 an address-literal may be [IPv4-address-literal] with no tag, so user@[192.168.1.1] is a valid mailbox host.

Currently mailto:user@[192.168.1.1] is not linkified, and with fuzzyIP enabled it is worse: the scheme and local part are dropped and a bare 192.168.1.1 IP link is returned instead. This adds the missing get_ipv4_mail_host() sibling and wires it into get_mail_host()/get_fuzzy_mail_host(), mirroring the existing IPv6 handling.

@puzrin

puzrin commented Sep 3, 2026

Copy link
Copy Markdown
Member

What is the added value of this? Do you have such links in the real world?

@spokodev

spokodev commented Sep 7, 2026

Copy link
Copy Markdown
Author

Fair question. The case is user@[192.168.1.1] — the bracketed address literal from RFC 5321 §4.1.3, not a bare user@192.168.1.1.

The value is consistency rather than new capability: linkify-it already matches user@[IPv6:2001:db8::1] via get_ipv6_mail_host, so bracketed address literals are already part of what it recognises — just only for the rarer of the two address types. This mirrors that branch and reuses the existing get_ipv4_addr, so it is ten lines and adds no new regex surface.

In prose both forms are rare, I will not pretend otherwise. If you would rather drop the IPv6 branch than mirror it, say so and I will close this.

@puzrin

puzrin commented Sep 9, 2026 •

Copy link
Copy Markdown
Member

This package uses heuristic. That means, real world examples are mandatory. Blind spec following just adds the noise for code and maintainability. IPv6 patch was accepted, because specifying IPv6 link was impossible without it. Email changes were probably not necessary, but i had no time to investigate this more deep and polish.

So, let's close this as "not needed".

For the future, I'd suggest to always create issue with reasons first. Because coding is not a problem now, but cost of PR review is much higher than discussing the topic in issue.

@puzrin puzrin closed this Sep 9, 2026
@spokodev

Copy link
Copy Markdown
Author

Understood, and thanks for spelling out the reasoning — "heuristic, so real-world examples are mandatory" is the part I had wrong. I was arguing from the IPv6 branch as precedent, and you are saying that branch is itself closer to an exception than a rule, so consistency with it is not an argument.

Point about issues taken too: I will open one with the case first next time rather than spending your review budget on a patch.

@puzrin

puzrin commented Sep 10, 2026

Copy link
Copy Markdown
Member

Could you explain the goals of your activity? Are accepted PR-s important for your needs?

Let me explain. I see intensive AI tools use. That's not bad in general, but has it's own specific - "frequent missing of project context". If that's landed as issues only - that's ideal for me (reasonably cheap processing + cheap ai-assisted bugfixes at my side).

But I understand you may need more than just filling issues. If you explain your preferences - I will try to suggest something, when both sides win and collaboration stays as efficient as possible.

@spokodev

Copy link
Copy Markdown
Author

No, accepted PRs are not the point. I am not collecting credit and I am not building a portfolio out of this.

The goal is narrower than that: find real defects in software a lot of people depend on, and get them fixed. Whether the fix carries my name, or you write it yourself in half the time because you know the codebase, makes no difference to me. argparse #190 is the good example — you shipped a better fix than mine in 3.0.2 the same day, and that is the outcome I wanted.

On the AI point: yes, it is AI-assisted, and I review and decide what gets opened. The context gap you describe is real, and it is mine to close, not the tool's. linkify-it #122 was exactly that — I brought an RFC to a heuristic library with no real-world example. That was my mistake, not a disagreement.

So: issues only in your repos from now on, no problem at all. I will keep reading the code and report what I find that way. If you would rather have a patch for some particular class of thing, tell me and I will.

And if you had something else in mind, propose it — I will work with whatever is cheapest for you.

@puzrin

puzrin commented Sep 21, 2026

Copy link
Copy Markdown
Member

Thank you! I'm glad to know we have no conflicting bases - that's important.

Then I suggest:

  • Create the issue first, with a focus on the problem.
  • Optionally, if you wish, add a PR if you don't mind the high chance of rejection. I don't reject PR's just because they are AI-generated Pick zlib's Z_FIXED fix in block type selection nodeca/pako#316. My choice is: "How can I solve the issue right now in 10 minutes and move forward?"

Notes about context. I usually ask AI these questions before reading anything more carefully:

  • Do users need this feature/fix? If not, can it be useful in the future?
  • What is the added value of this feature/fix? Can users get it on their side easily without a mainstream change? (~ the same as previous, but from a different view angle)
  • For the PR, is the suggested fix optimal, or can it be done better?

These questions don't add context automatically. But they're cheap to run and remove a lot of noise.

@spokodev

Copy link
Copy Markdown
Author

Agreed — and thank you for writing that out instead of just closing the tab.

Issue first in your repos from now on. A PR only when I think it survives your questions, and I won't mind when one doesn't.

Your three questions now sit in front of my filing step: does a user need this; what is the added value, and can a user get it on their side without a change upstream; and for a PR, is this fix optimal or can it be done better. The third is the one I was quietly skipping. I was checking that my patch worked — never that it was the best patch on offer, which is a much less comfortable question to ask yourself.

#122 fails the first two outright, so under this it would never have left the house.

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.

2 participants