Skip to content
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

Make ChangeTracker#newLineCharacter public, to avoid having to pass newLineCharacter around as a parameter #20574

Merged
2 commits merged into from
Dec 12, 2017

Conversation

ghost
Copy link

@ghost ghost commented Dec 8, 2017

Noticed in many code fixes that we have functions with both a ChangeTracker and newLineCharacter as parameters. ChangeTracker already stores newLineCharacter as a property, so we can just make that property public (still internal though) and access that.

@ghost ghost requested a review from amcasey December 8, 2017 15:09
@amcasey
Copy link
Member

amcasey commented Dec 8, 2017

I agree that it would be nice to track the newline character in fewer places, but it's not intuitively obvious to me why the ChangeTracker would be the appropriate place to keep the definitive copy. It looks like it's already on the context (twice?) and, in fact, that's where the change tracker gets it. Other than some cleanup in insertNodeAtConstructorStart and insertNodeAtClassStart, I'm not sure how this change improves consistency or reduces redundancy.

@ghost ghost force-pushed the changeTracker_newLineCharacter branch from 1b86c76 to 9b769cc Compare December 11, 2017 22:50
@ghost
Copy link
Author

ghost commented Dec 11, 2017

The idea was that ChangeTracker should be the one responsible for dealing with text, while an individual refactor / code fix should only be responsible for dealing with nodes. This still wasn't the case, though, if they needed to access newLineCharacter. So I just pushed a new commit changing ChangeTracker's methods to not take newLineCharacter at all, thus it can be private again.

In addition the following are now only used publicly by tests: useNonAdjustedStartPosition, useNonAdjustedEndPosition, prefix, and suffix, and the replaceNodeRange method. We should consider just removing those tests if this is functionality we don't need to be public. The user of ChangeTracker shouldn't care about these, ChangeTracker should be able to figure out what options are appropriate given the nodes involved.

Copy link
Member

@amcasey amcasey left a comment

Choose a reason for hiding this comment

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

Nice! I like this approach much better. (I did not carefully review the new textChanges functions.)

@ghost ghost merged commit 8ad4aee into master Dec 12, 2017
@ghost ghost deleted the changeTracker_newLineCharacter branch December 12, 2017 20:23
@ghost ghost mentioned this pull request Mar 5, 2018
@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.

1 participant