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

Allow mulitiple edits at the same offset. #158

Closed
wants to merge 1 commit into from

Conversation

dantleech
Copy link
Contributor

@dantleech dantleech commented Jul 8, 2017

Currently an Assertion error is thrown when text edits are made in the wrong order AND when multiple edits are made in the same place. Which I believe is a mistake as this works as expected until I run it in an environment with assertions turned on:

$source = '<?php';
$textEdits[] = new TextEdit(0, 0, 'Hello');
$textEdits[] = new TextEdit(0, 0, 'World');
echo TextEdit::applyEdits($textEdits, $source);

Expected:

HelloWorld<?php

Actual: Assertion error is thrown.

If assertions are disabled (zend.assertions = -1) this works as expected.

In addtion I changed the assertion to an OutOfBounds exception due to the inconsitency of the behavior of assert.

@msftclas
Copy link

msftclas commented Jul 8, 2017

@dantleech,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

@roblourens
Copy link
Member

Why would you expect HelloWorld and not WorldHello? I would think that Hello would be inserted at 0, then World inserted at 0. I think this is why that limitation is there in the first place - it leads to confusing situations.

@dantleech
Copy link
Contributor Author

dantleech commented Jul 17, 2017

I don't know if its good to have the limitation or not - I suppose it can be worked around by building up all of the edits at that offset into a single edit.

I do think that an exception would be better than assert in this case (as an exception will always be thrown whereas assert is env. specific and can lead to confusion).

@roblourens
Copy link
Member

roblourens commented Jul 17, 2017

I don't know why one check is an assert, and the other is an exception. I think it would make sense to make the assert an exception as well. #161

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants