Skip to content

Conversation

ghost
Copy link

@ghost ghost commented Jul 7, 2017

Should wait on #17017 and be rebased once that's in.
These functions were only used in one place, and the code is simpler if we just directly do the work instead of using two separate functions.

@ghost ghost requested a review from sheetalkamat July 7, 2017 21:45
@ghost ghost force-pushed the childFrom branch from ed49274 to e94f185 Compare July 10, 2017 18:36
@ghost ghost force-pushed the childFrom branch from e94f185 to 67b19f4 Compare July 10, 2017 22:11
@ghost
Copy link
Author

ghost commented Jul 12, 2017

@sheetalkamat PR is ready now.

zeroBasedColumn: childInfo.relativePosition,
lineText: childInfo.child.text,
};
Debug.assert(this.children.length !== 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it guaranteed that children will never be empty when this is called? The previous logic had a specific case to handle that (returning child as undefined from childFromCharOffset).

Copy link
Author

Choose a reason for hiding this comment

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

It looks like that is possible for a LineNode to have 0 children if this document is the empty string. Added a test.

@rbuckton
Copy link
Contributor

rbuckton commented Aug 8, 2017

Your test is failing.

@ghost ghost force-pushed the childFrom branch from cfc7c59 to ec357db Compare August 8, 2017 21:23
@ghost
Copy link
Author

ghost commented Aug 8, 2017

Fixed.

@ghost ghost merged commit b8c37bb into master Aug 9, 2017
@ghost ghost deleted the childFrom branch August 9, 2017 20:43
uniqueiniquity pushed a commit to uniqueiniquity/TypeScript that referenced this pull request Aug 9, 2017
* Inline childFromLineNumber and childFromCharOffset

* Handle empty document -- root node with 0 children

* Fix test
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 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.

2 participants