-
-
Notifications
You must be signed in to change notification settings - Fork 84
Make BaseTarget parameters getters stricter #1534
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 |
---|---|---|
|
@@ -15,20 +15,32 @@ import { | |
createContinuousRangeUntypedTarget, | ||
} from "../targetUtil/createContinuousRange"; | ||
|
||
/** Parameters supported by most target classes */ | ||
export interface CommonTargetParameters { | ||
/** Parameters supported by all target classes */ | ||
export interface MinimumTargetParameters { | ||
readonly editor: TextEditor; | ||
readonly isReversed: boolean; | ||
readonly contentRange: Range; | ||
readonly thatTarget?: Target; | ||
} | ||
|
||
/** Parameters supported by most target classes */ | ||
export interface CommonTargetParameters extends MinimumTargetParameters { | ||
readonly contentRange: Range; | ||
} | ||
|
||
export interface CloneWithParameters { | ||
readonly thatTarget?: Target; | ||
readonly contentRange?: Range; | ||
} | ||
|
||
export default abstract class BaseTarget implements Target { | ||
/** | ||
* An abstract target. All targets subclass this. | ||
* | ||
* @template TParameters The constructor parameters. | ||
*/ | ||
export default abstract class BaseTarget< | ||
in out TParameters extends MinimumTargetParameters, | ||
> implements Target | ||
{ | ||
protected readonly state: CommonTargetParameters; | ||
isLine = false; | ||
isToken = true; | ||
|
@@ -39,7 +51,7 @@ export default abstract class BaseTarget implements Target { | |
isNotebookCell = false; | ||
isWord = false; | ||
|
||
constructor(parameters: CommonTargetParameters) { | ||
constructor(parameters: TParameters & CommonTargetParameters) { | ||
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. This ensures that whatever type is declared actually ends up existing in the subclass constructor somehow. 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. oh really? so the type signature of the base class constructor influences the type signature of derived class constructors? That's surprising to me. And I'm not sure how that works in the case where the derived class uses minimum parameters instead of common parameters |
||
this.state = { | ||
editor: parameters.editor, | ||
isReversed: parameters.isReversed, | ||
|
@@ -112,16 +124,16 @@ export default abstract class BaseTarget implements Target { | |
throw new NoContainingScopeError("boundary"); | ||
} | ||
|
||
readonly cloneWith = (parameters: CloneWithParameters) => { | ||
private cloneWith(parameters: CloneWithParameters) { | ||
const constructor = Object.getPrototypeOf(this).constructor; | ||
|
||
return new constructor({ | ||
...this.getCloneParameters(), | ||
...parameters, | ||
}); | ||
}; | ||
} | ||
|
||
protected abstract getCloneParameters(): object; | ||
protected abstract getCloneParameters(): TParameters; | ||
|
||
createContinuousRangeTarget( | ||
isReversed: boolean, | ||
|
@@ -170,9 +182,8 @@ export default abstract class BaseTarget implements Target { | |
* | ||
* @returns The object to be used for determining equality | ||
*/ | ||
protected getEqualityParameters(): object { | ||
const { thatTarget, ...otherCloneParameters } = | ||
this.getCloneParameters() as { thatTarget?: Target }; | ||
protected getEqualityParameters(): Omit<TParameters, "thatTarget"> { | ||
const { thatTarget, ...otherCloneParameters } = this.getCloneParameters(); | ||
|
||
return { | ||
...otherCloneParameters, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,7 +9,7 @@ export interface SubTokenTargetParameters extends CommonTargetParameters { | |
readonly trailingDelimiterRange?: Range; | ||
} | ||
|
||
export default class SubTokenWordTarget extends BaseTarget { | ||
export default class SubTokenWordTarget extends BaseTarget<SubTokenTargetParameters> { | ||
private leadingDelimiterRange_?: Range; | ||
private trailingDelimiterRange_?: Range; | ||
insertionDelimiter: string; | ||
|
@@ -43,11 +43,12 @@ export default class SubTokenWordTarget extends BaseTarget { | |
return getDelimitedSequenceRemovalRange(this); | ||
} | ||
|
||
protected getCloneParameters() { | ||
protected getCloneParameters(): SubTokenTargetParameters { | ||
return { | ||
...this.state, | ||
leadingDelimiterRange: this.leadingDelimiterRange_, | ||
trailingDelimiterRange: this.trailingDelimiterRange_, | ||
insertionDelimiter: this.insertionDelimiter, | ||
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. All this ceremony wasn't for nothing, it seems! I added the return type here for autocomplete. 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. So you don't get autocomplete for free from the generic at the top / base class function? Not that I'm opposed to extra return type annotations 🤷♂️ |
||
}; | ||
} | ||
} |
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.
Can you explain why you used
in out
here?