-
Notifications
You must be signed in to change notification settings - Fork 133
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
Add optional absolute char indexing for highlighting #2584
base: master
Are you sure you want to change the base?
Changes from 5 commits
e4f0e2b
78c8e84
46901a3
f679edb
fe14d75
7cb451b
2245743
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 |
---|---|---|
@@ -1,6 +1,6 @@ | ||
import { splitCodeAndIndentation } from './helper'; | ||
|
||
const LINESLICE_CHAR_REGEX = /(\d+)\[(\d*):(\d*)]/; | ||
const LINESLICE_CHAR_REGEX = /(_?)(\d+)\[(\d*):(\d*)]/; | ||
const LINESLICE_WORD_REGEX = /(\d+)\[(\d*)::(\d*)]/; | ||
const LINEPART_REGEX = /(\d+)\[(["'])((?:\\.|[^\\])*?)\2]/; | ||
const UNBOUNDED = -1; | ||
|
@@ -43,22 +43,29 @@ export class HighlightRuleComponent { | |
const linesliceWordMatch = compString.match(LINESLICE_WORD_REGEX); | ||
const sliceMatch = linesliceCharMatch || linesliceWordMatch; | ||
if (sliceMatch) { | ||
// There are four capturing groups: [full match, line number, start bound, end bound] | ||
// There are four/five capturing groups: [full match, is highlight spaces (for char slice), | ||
// line number, start bound, end bound] | ||
const groups = sliceMatch.slice(1); // discard full match | ||
|
||
let isHighlightSpaces = false; | ||
if (sliceMatch === linesliceCharMatch) { | ||
isHighlightSpaces = groups.shift() === '_'; | ||
} | ||
|
||
const lineNumber = HighlightRuleComponent | ||
.isValidLineNumber(groups.shift() ?? '', 1, lines.length, lineNumberOffset); | ||
if (!lineNumber) return null; | ||
|
||
const isUnbounded = groups.every(x => x === ''); | ||
|
||
if (isUnbounded) { | ||
return new HighlightRuleComponent(lineNumber, true, []); | ||
} | ||
|
||
let bound = groups.map(x => (x !== '' ? parseInt(x, 10) : UNBOUNDED)) as [number, number]; | ||
const isCharSlice = sliceMatch === linesliceCharMatch; | ||
bound = isCharSlice | ||
? HighlightRuleComponent.computeCharBounds(bound, lines[lineNumber - 1]) | ||
? HighlightRuleComponent.computeCharBounds(bound, lines[lineNumber - 1], isHighlightSpaces) | ||
: HighlightRuleComponent.computeWordBounds(bound, lines[lineNumber - 1]); | ||
|
||
return new HighlightRuleComponent(lineNumber, true, [bound]); | ||
|
@@ -95,38 +102,50 @@ export class HighlightRuleComponent { | |
* comparing the bounds and the line's range. | ||
* | ||
* If the bound does not specify either the start or the end bound, the computed bound will default | ||
* to the start or end of line, excluding leading whitespaces. | ||
* to the start or end of line. The bound will either include or exclude leading whitespaces depending | ||
* on the user's preference. | ||
* | ||
* @param bound The user-defined bound | ||
* @param line The given line | ||
* @param isHighlightSpaces Whether to highlight leading whitespaces | ||
* @returns {[number, number]} The actual bound computed | ||
*/ | ||
static computeCharBounds(bound: [number, number], line: string): [number, number] { | ||
static computeCharBounds(bound: [number, number], line: string, | ||
isHighlightSpaces: boolean): [number, number] { | ||
const [indents] = splitCodeAndIndentation(line); | ||
let [start, end] = bound; | ||
|
||
if (start === UNBOUNDED) { | ||
start = indents.length; | ||
} else { | ||
if (isHighlightSpaces) { | ||
start = 0; | ||
} else { | ||
start = indents.length; | ||
} | ||
} else if (!isHighlightSpaces) { | ||
start += indents.length; | ||
// Clamp values | ||
if (start < indents.length) { | ||
start = indents.length; | ||
} else if (start > line.length) { | ||
start = line.length; | ||
} | ||
} else if (start > line.length) { | ||
start = line.length; | ||
} | ||
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. The conditional logic is slightly convoluted for me here. If Also, given that Lines 129-130 and 132-133 are duplicated, I wonder if there is a way to simplify this chunk of code. We don't need to shy away from ternary conditional operators if they make the code easier to understand. 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.
In theory, it's possible for that to occur. For example, for
Thanks for the feedback! I'll see if there's a way to simplify it more to improve readability. |
||
|
||
if (end === UNBOUNDED) { | ||
end = line.length; | ||
} else { | ||
} else if (!isHighlightSpaces) { | ||
end += indents.length; | ||
|
||
// Clamp values | ||
if (end < indents.length) { | ||
end = indents.length; | ||
} else if (end > line.length) { | ||
end = line.length; | ||
} | ||
} else if (end > line.length) { | ||
end = line.length; | ||
} | ||
|
||
return [start, end]; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,28 +44,55 @@ describe('parseRuleComponent', () => { | |
}); | ||
}); | ||
|
||
describe('computeCharBounds', () => { | ||
describe('computeCharBounds, no whitespace highlighting', () => { | ||
test('computes character bounds correctly', () => { | ||
const bounds = HighlightRuleComponent.computeCharBounds([2, 5], ' some text'); | ||
const bounds = HighlightRuleComponent.computeCharBounds([2, 5], ' some text', false); | ||
expect(bounds).toEqual([4, 7]); | ||
}); | ||
|
||
test('handles unbounded start correctly', () => { | ||
const bounds = HighlightRuleComponent.computeCharBounds([-1, 4], ' some text'); | ||
const bounds = HighlightRuleComponent.computeCharBounds([-1, 4], ' some text', false); | ||
expect(bounds).toEqual([2, 6]); | ||
}); | ||
|
||
test('handles unbounded end correctly', () => { | ||
const bounds = HighlightRuleComponent.computeCharBounds([3, -1], ' some text'); | ||
const bounds = HighlightRuleComponent.computeCharBounds([3, -1], ' some text', false); | ||
expect(bounds).toEqual([5, ' some text'.length]); | ||
}); | ||
|
||
test('handles out-of-range bounds correctly', () => { | ||
const bounds = HighlightRuleComponent.computeCharBounds([30, 40], ' some text'); | ||
const bounds = HighlightRuleComponent.computeCharBounds([30, 40], ' some text', false); | ||
expect(bounds).toEqual([' some text'.length, ' some text'.length]); | ||
}); | ||
}); | ||
|
||
describe('computeCharBounds, with whitespace highlighting', () => { | ||
test('computes character bounds correctly', () => { | ||
const bounds = HighlightRuleComponent.computeCharBounds([2, 5], ' some text', true); | ||
expect(bounds).toEqual([2, 5]); | ||
}); | ||
|
||
test('handles unbounded start correctly', () => { | ||
const bounds = HighlightRuleComponent.computeCharBounds([-1, 4], ' some text', true); | ||
expect(bounds).toEqual([0, 4]); | ||
}); | ||
|
||
test('handles unbounded end correctly', () => { | ||
const bounds = HighlightRuleComponent.computeCharBounds([3, -1], ' some text', true); | ||
expect(bounds).toEqual([3, ' some text'.length]); | ||
}); | ||
|
||
test('handles out-of-range bounds correctly', () => { | ||
const bounds = HighlightRuleComponent.computeCharBounds([30, 40], ' some text', true); | ||
expect(bounds).toEqual([' some text'.length, ' some text'.length]); | ||
}); | ||
|
||
test('handles line-length end correctly', () => { | ||
const bounds = HighlightRuleComponent.computeCharBounds([0, 4], ' abcd', true); | ||
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. Why 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. There's no underlying reason for |
||
expect(bounds).toEqual([0, 4]); | ||
}); | ||
}); | ||
|
||
describe('computeWordBounds', () => { | ||
test('computes word bounds correctly', () => { | ||
const bounds = HighlightRuleComponent.computeWordBounds([1, 2], ' some text here'); | ||
|
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.