Skip to content

Require a name and a domain in email addresses - #34

Open
sergical wants to merge 1 commit into
mainfrom
fix/email-require-domain
Open

sergical wants to merge 1 commit into
mainfrom
fix/email-require-domain

Conversation

@sergical

Copy link
Copy Markdown
Member

isEmail now needs some text before the @, a domain with a dot after it, and no spaces. Before, any string with an @ passed, so "@" and "name@" were treated as valid addresses.

Created with Claude Code

@github-actions

Copy link
Copy Markdown

Verdict: Approve with minor suggestions — the regex is a solid improvement over the old includes("@") check, but has a couple of known edge-case gaps and could use documentation.

Correctness

  • No failing tests (run_tests found no related tests to run for this fixture).
  • Improvement: correctly rejects strings with multiple @ symbols, missing domain dots, and leading/trailing whitespace around the whole string — all of which the old includes("@") check incorrectly accepted.
  • Edge case / behavior change to be aware of: addresses without a dot in the domain (e.g. user@localhost) now return false, whereas they used to pass. If any caller or test relies on such addresses being accepted, this is a breaking change worth calling out in the PR description.
  • Superficial validation gap: the regex only requires some dot after @, it doesn't prevent malformed patterns like consecutive dots or a trailing dot in the domain (e.g. [email protected], [email protected]. would still match). Not a regression, but worth knowing this isn't full RFC validation.
  • Unicode/IDN domains are still accepted by [^\s@]+ (consistent with previous lenient behavior, not a new issue).

Style

  • The regex is opaque inline; consider extracting it to a named module-level constant (e.g. const EMAIL_PATTERN = ...) both for readability and to avoid re-constructing the literal on every call (minor; JS regex literals are typically cached by engines, but naming still aids clarity).
  • Add a short comment noting this is a basic format check, not full RFC 5322 validation, so readers don't assume stronger guarantees than it provides.
  • Optional: consider a doc comment on isEmail clarifying its intentionally lightweight scope.

Overall a reasonable, low-risk tightening of validation logic; the above are non-blocking suggestions.

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.

1 participant