-
Notifications
You must be signed in to change notification settings - Fork 4
fix(parser): retain single-letter language skills #855
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -79,6 +79,12 @@ describe("tokenizeSkillLine", () => { | |||||||||||||||||||||||||||||||||||||||||||||
| expect(tokenizeSkillLine(",,,;;;")).toEqual([]); | ||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| it("keeps the defensible single-letter languages and rejects stray glyphs (#832)", () => { | ||||||||||||||||||||||||||||||||||||||||||||||
| const result = tokenizeSkillLine("C, R, D, X"); | ||||||||||||||||||||||||||||||||||||||||||||||
| expect(result).toEqual(expect.arrayContaining(["C", "R", "D"])); | ||||||||||||||||||||||||||||||||||||||||||||||
| expect(result).not.toContain("X"); | ||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+82
to
+86
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Secondary (finding 2). The behaviour is right — I ran #832's whole AC token set against this head and every row passes. The gap is that only the
Verified outputs, so the suggestion below is asserting real behaviour rather than a guess:
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| it("drops the whole cell when a URL is present in a comma-separated list", () => { | ||||||||||||||||||||||||||||||||||||||||||||||
| // tokenizeCell's looksLikeContactLink check fires on the ENTIRE cleaned | ||||||||||||||||||||||||||||||||||||||||||||||
| // cell before the split. "github.com/janesmith" matches the path-slash | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -130,6 +130,12 @@ const PROFILE_HOST_RE = | |
| * "Socket.io", "ASP.NET") that has no slash. */ | ||
| const URLISH_RE = /(https?:\/\/|www\.|\b[a-z0-9-]+\.[a-z]{2,}\/\S)/i; | ||
|
|
||
| /** One-character tokens that are real, commonly-listed languages. The length | ||
| * floor in `isSkillToken` is a noise guard against stray glyphs left by column | ||
| * splitting; these are the only single characters that are not noise, so they | ||
| * are allowlisted rather than lowering the floor. */ | ||
| const SINGLE_LETTER_SKILLS = new Set(["c", "r", "d"]); | ||
|
|
||
| /** True when a candidate skill token is really a professional-profile link | ||
| * (GitHub / LinkedIn / portfolio, etc.) or its bare heading word. Such links | ||
| * belong only in the contact/profile section, never in Skills. */ | ||
|
|
@@ -139,6 +145,7 @@ function looksLikeContactLink(tok: string): boolean { | |
| } | ||
|
|
||
| function isSkillToken(tok: string): boolean { | ||
| if (tok.length === 1 && SINGLE_LETTER_SKILLS.has(tok.toLowerCase())) return true; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirming this line rather than flagging it: the early |
||
| if (tok.length < 2 || tok.length > 40) return false; | ||
| if (/^\d+$/.test(tok)) return false; | ||
| // A professional-profile link (or its bare "GitHub" / "LinkedIn" heading) is | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -58,11 +58,5 @@ | |||||||||||||||||||||
| "Raspberry Pi", | ||||||||||||||||||||||
| "iOS" | ||||||||||||||||||||||
| ], | ||||||||||||||||||||||
| "knownWrong": { | ||||||||||||||||||||||
| "skills": { | ||||||||||||||||||||||
| "issue": null, | ||||||||||||||||||||||
| "status": "unfiled", | ||||||||||||||||||||||
| "note": "The single-letter token “C” is DROPPED from the Languages row while “C++” survives. Second independent measurement of the same drop (see google-docs/google-docs-skia-proxy-role-first-experience)." | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| "knownWrong": {} | ||||||||||||||||||||||
|
Comment on lines
60
to
+61
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit (finding 5), non-blocking. With the entry resolved, this leaves the only empty
Suggested change
|
||||||||||||||||||||||
| } | ||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking (finding 1). The measured unfiled count on this head is 7, not 8 —
npm run check:baselineslists exactly seven⚠ … status "unfiled"entries after this PR resolves the two C-drop ones.main's9was tight against 9 real entries, so resolving two lands on 7.Leaving it at
8keeps the assertion green but opens one free slot, which contradicts this constant's own docblock three lines above ("undescribed debt may not GROW") — the next PR could add a brand-new unfiled disagreement and this gate would not notice.#832 step 4 asks for a lowering by 2 and says to take the LOWER number when a sibling issue has already moved it; a sibling moved
10 → 9before this branch, which is why the literal8from the issue text is now one too high.I'm leaving this as prose rather than a one-click suggestion because it changes what a gate asserts — that's outside the bound on reviewer-applied fixes, even at one character.