Skip to content

Commit 507284f

Browse files
authored
fix(url): address code review feedback
- Remove underscore from fullHostParser label character classes (not valid in standard DNS labels per RFC 1123) - Improve trimUrlSuffix bracket counting: use open/close occurrence counts instead of simple presence check (correctly handles cases like (foo](bar) where same-type counts matter) - Add value assertions to port rejection tests
1 parent f3dd741 commit 507284f

2 files changed

Lines changed: 21 additions & 5 deletions

File tree

src/URL.ts

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -57,10 +57,22 @@ type URLOutputs = {
5757
parser: URLEntity;
5858
};
5959

60+
/**
61+
* Count occurrences of a single character in a string.
62+
*/
63+
function countChar(s: string, ch: string): number {
64+
let n = 0;
65+
for (let i = 0; i < s.length; i++) {
66+
if (s[i] === ch) n++;
67+
}
68+
return n;
69+
}
70+
6071
/**
6172
* Trim trailing characters from a URL suffix that are clearly unmatched
6273
* sentence/context punctuation rather than part of the URL itself.
63-
* Balanced brackets are kept (e.g. "(foo)" is preserved).
74+
* Balanced brackets are kept by counting opens vs closes (e.g. "(foo)" is
75+
* preserved, but a lone ")" is trimmed).
6476
*/
6577
function trimUrlSuffix(s: string): string {
6678
const closingPairs: Record<string, string> = { ")": "(", "]": "[", "}": "{" };
@@ -70,7 +82,9 @@ function trimUrlSuffix(s: string): string {
7082
if (".,;!?".includes(last)) {
7183
result = result.slice(0, -1);
7284
} else if (last in closingPairs) {
73-
if (!result.includes(closingPairs[last])) {
85+
const opener = closingPairs[last];
86+
// Trim if there are more closing brackets than opening ones
87+
if (countChar(result, last) > countChar(result, opener)) {
7488
result = result.slice(0, -1);
7589
} else {
7690
break;
@@ -109,10 +123,10 @@ const fullHostParser: Parser<string> = (ctx) => {
109123
}
110124

111125
// Hostname: one or more DNS labels separated by dots.
112-
// A label starts and ends with a letter, digit, or Unicode char and
126+
// A label starts and ends with a letter or digit (Unicode included) and
113127
// may contain hyphens internally (RFC 1123 + IDN).
114-
const isLabelStart = (ch: string): boolean => /[\p{L}\p{N}_]/u.test(ch);
115-
const isLabelMid = (ch: string): boolean => /[\p{L}\p{N}_\-]/u.test(ch);
128+
const isLabelStart = (ch: string): boolean => /[\p{L}\p{N}]/u.test(ch);
129+
const isLabelMid = (ch: string): boolean => /[\p{L}\p{N}\-]/u.test(ch);
116130

117131
const parseLabelEnd = (pos: number): number => {
118132
if (pos >= text.length || !isLabelStart(text[pos])) return -1;

tests/URL.test.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -225,10 +225,12 @@ Deno.test("URL rejects decimal port :1.5", () => {
225225
const res = Duckling().extract("http://example.com:1.5");
226226
assertEquals(res[0].kind, "url");
227227
assertEquals(res[0].text, "http://example.com");
228+
assertEquals(res[0].value, { url: "http://example.com" });
228229
});
229230

230231
Deno.test("URL rejects out-of-range port :65536", () => {
231232
const res = Duckling().extract("http://example.com:65536 end");
232233
assertEquals(res[0].kind, "url");
233234
assertEquals(res[0].text, "http://example.com");
235+
assertEquals(res[0].value, { url: "http://example.com" });
234236
});

0 commit comments

Comments
 (0)