fix: allow labels to have same synonym #95
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Assessment against linked issues
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/scraper/text.spec.ts (1)
33-41: LGTM! The new test case effectively verifies the multiple-labels-per-synonym functionality.The test is well-structured and clearly demonstrates that multiple labels can now share the same synonym.
Consider adding these additional test cases to improve coverage:
+ test('should handle mixed scenario with shared and unique synonyms', () => { + const body = 'Body with labels: Synonym1 Synonym2' + const labels = ['Label1', 'Label2', 'Label3'] + const labelsSynonyms = { + Label1: ['Synonym1'], + Label2: ['Synonym1'], + Label3: ['Synonym2'] + } + + const result = getIssueLabels(body, labels, labelsSynonyms) + + expect(result).toEqual(['Label1', 'Label2', 'Label3']) + }) + test('should handle case-insensitive synonym matching', () => { + const body = 'Body with labels: SYNONYM1 synonym1' + const labels = ['Label1', 'Label2'] + const labelsSynonyms = { + Label1: ['Synonym1'], + Label2: ['Synonym1'] + } + + const result = getIssueLabels(body, labels, labelsSynonyms) + + expect(result).toEqual(['Label1', 'Label2']) + })src/scraper/text.ts (3)
28-31: LGTM! Consider performance optimization for large datasets.The implementation correctly allows multiple labels per synonym by accumulating them in an array. The spread operator with fallback to empty array handles the initial case well.
For large datasets, consider pre-allocating the arrays to avoid multiple spreads:
- synonymsObject[synonym.toLowerCase()] = [ - ...(synonymsObject[synonym.toLowerCase()] || []), - label - ] + const key = synonym.toLowerCase(); + if (!synonymsObject[key]) { + synonymsObject[key] = []; + } + synonymsObject[key].push(label);
Line range hint
36-41: LGTM! Consider adding edge case tests.The change to
flatMapcorrectly handles the new array-based synonym structure while maintaining the fallback chain.Consider adding test cases for:
- Multiple synonyms matching in the same line
- Case sensitivity handling
- Non-matching synonyms fallback behavior
Line range hint
1-63: Architecture looks solid and meets requirements.The changes effectively solve issue #93 by:
- Changing the data structure to support multiple labels per synonym
- Maintaining case-insensitive matching
- Preserving existing fallback behavior
- Using appropriate functional programming patterns
The solution is clean, maintainable, and scales well.
Consider documenting the synonym-to-multiple-labels relationship in the
auto-label.json5schema to help users understand the new capability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/scraper/text.spec.ts(1 hunks)src/scraper/text.ts(1 hunks)
🔇 Additional comments (2)
src/scraper/text.spec.ts (1)
33-41: Verify the implementation in text.ts matches the test expectations
Let's ensure the implementation correctly handles multiple labels with the same synonym.
✅ Verification successful
Implementation correctly handles multiple labels with the same synonym
The implementation in text.ts matches the test expectations perfectly. Here's why:
- The
compareLabelsfunction creates asynonymsObjectthat maps each synonym to an array of corresponding labels - When a synonym is found in the text, it returns all the labels associated with that synonym through the
synonymsObject[elem.toLowerCase()]lookup - The test case verifies this behavior by checking that when 'Synonym1' is found in the text, both 'Label1' and 'Label2' are returned since they both have 'Synonym1' as a synonym
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation in text.ts
# Check the type definition change and implementation
ast-grep --pattern 'type $_ = Record<string, string[]>'
# Check the compareLabels function implementation
ast-grep --pattern 'function compareLabels($$$) {
$$$
}'
# Look for any related functions that might need updates
rg -A 5 "getIssueLabels|compareLabels" src/scraper/text.ts
Length of output: 659
Script:
#!/bin/bash
# Let's get the complete implementation of compareLabels to verify the logic
rg -A 15 'compareLabels = \(' src/scraper/text.ts
# Also check for any test cases that might help understand the behavior
rg -A 10 'test\(.*Synonym' src/scraper/text.spec.ts
Length of output: 626
Script:
#!/bin/bash
# Let's try a different approach to get the complete implementation
ast-grep --pattern 'const compareLabels = ($$$) => {
$$$
}'
# And get the complete test file content
cat src/scraper/text.spec.ts
Length of output: 1498
Script:
#!/bin/bash
# Let's get the complete implementation file
cat src/scraper/text.ts
Length of output: 1714
src/scraper/text.ts (1)
25-25: LGTM! Verify type change impact across codebase.
The type change from Record<string, string> to Record<string, string[]> correctly enables multiple labels per synonym as intended.
Let's verify the impact of this type change:
✅ Verification successful
Type change is safe and already aligned across codebase
The type change from Record<string, string> to Record<string, string[]> is safe as all related code already expects and handles string arrays:
labelsSynonymsis consistently typed asRecord<string, string[]>across the codebase in:- Configuration types (
src/domain/getConfigFile.ts) - Function parameters (
src/scraper/text.ts) - Test files (
src/domain/getLabelConfigs.spec.ts)
- Configuration types (
- The implementation in
compareLabelsalready handles arrays through spread operations and array methods - Test cases demonstrate usage with array values (
src/scraper/text.spec.ts)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of synonymsObject or labelsSynonyms to ensure type compatibility
rg -t typescript "Record<string,\s*string(\[\])?" --glob '!src/scraper/text.ts'
Length of output: 116
Script:
#!/bin/bash
# Search for usages of synonymsObject or similar patterns in TypeScript files
rg -l -g "*.ts" "Record<string,\s*string"
# Search for direct usage of synonymsObject
rg -g "*.ts" "synonymsObject" -A 2 -B 2
# Search for similar patterns with labelsSynonyms
rg -g "*.ts" "labelsSynonyms" -A 2 -B 2
Length of output: 5357
Renato66
deleted the
93-unable-to-add-same-synonym-to-different-labels
branch
closes #93