Skip to content

Conversation

ghost
Copy link

@ghost ghost commented Mar 22, 2018

  • We don't use ConfigurableStartEnd after the change is created, so those just need to store InsertNodeOptions.
  • The useIndentationFromFile option was never used.

@ghost ghost requested a review from amcasey March 22, 2018 22:57

// TODO (https://github.com/Microsoft/TypeScript/issues/21246): default should probably be useNonAdjustedPositions
public replaceRange(sourceFile: SourceFile, range: TextRange, newNode: Node, options: ChangeNodeOptions = {}) {
public replaceRange(sourceFile: SourceFile, range: TextRange, newNode: Node, options: InsertNodeOptions = {}) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As before, I personally find it strange to change parameter types of some, but not all, members of this family of functions.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interface ChangeNodeOptions extends ConfigurableStartEnd, InsertNodeOptions {}. ConfigurableStartEnd determines how the start and end positions of the node parameter are adjusted. This function doesn't take a node as a parameter, so those options would be unused.

}

export interface ChangeNodeOptions extends ConfigurableStartEnd, InsertNodeOptions {
readonly useIndentationFromFile?: boolean;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gather this property was unused?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right.

@ghost ghost merged commit a9d65b8 into master Mar 26, 2018
@ghost ghost deleted the textChanges_interfaces branch March 26, 2018 22:35
@microsoft microsoft locked and limited conversation to collaborators Jul 25, 2018
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant