Skip to content
This repository was archived by the owner on Mar 1, 2019. It is now read-only.

Alternative support for len field in ReplaceText event #14

Merged
merged 4 commits into from
May 2, 2017
Merged

Alternative support for len field in ReplaceText event #14

merged 4 commits into from
May 2, 2017

Conversation

albel727
Copy link
Contributor

This reverts #12 and #13 made in preparation of rust-lang/rls#281.

And further adds code necessary for rust-lang/rls#283.

nrc
nrc reviewed May 2, 2017
View reviewed changes
Copy link
Member

@nrc nrc left a comment

Choose a reason for hiding this comment

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

Looks good, I think this approach looks better. I'd just like an extra comment on the new field before merging.

src/lib.rs Outdated
@@ -39,6 +39,7 @@ pub enum Change {
/// Changes in-memory contents of the previously added file.
ReplaceText {
span: Span,
len: Option<u64>,
Copy link
Member

Choose a reason for hiding this comment

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

Could you comment on when/how this field is used

@albel727
Copy link
Contributor Author

albel727 commented May 2, 2017

Documented fields in ReplaceText and rebased on top of master to get rid of the Revert "Bump rls-span to 0.2" commit, which was introduced by another pull request.

albel727 added 2 commits May 2, 2017 16:09
@albel727
Document fields in ReplaceText event
16b28d0
@albel727
v0.2
f4e8714 
Add `len` Option field to ReplaceText event,
as an alternative to specifying row_end/col_end of replaced text.

This is necessary to better support Emacs, which has a hard time
calculating the latter.
@nrc nrc merged commit e41acd8 into rust-dev-tools:master May 2, 2017
@albel727
Copy link
Contributor Author

albel727 commented May 2, 2017

Oh damn, I didn't notice there were tests too, sorry. I assumed there's none due to lack of Travis setup.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@nrc nrc nrc left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@albel727 @nrc