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

Fix off-by-one on maxlLine calc #573

Merged

Conversation

rkeithhill
Copy link
Contributor

The old code allowed the line param to be equal to FileLines.Count + 1.
That's not a valid index even after subtracting 1 to get to a 0-based index.
Also added more data to log messages to help in the future.

@daviwil I pretty sure about the off-by-one but not 100% sure. Can you double check? At the very least, I'd like to enhance the log messages.

This is related to #567. I'm not sure if this fixes it (kind of doubt it) but I'm hopeful the enhanced log messages might help if someone repro's this.

The old code allowed the line param to be *equal* to FileLines.Count + 1.
That's not a valid index even after subtracting 1 to get to 0-based indices.
Also added more data to log messages to help in the future.
@rkeithhill rkeithhill requested a review from daviwil November 5, 2017 04:38
@rkeithhill
Copy link
Contributor Author

So how are you running the unit tests these days? I can only build PSES by building the extension from VSCode. When I try to run the unit tests in VS, it tries to build and the build fails (even though it builds from VSCode).

@daviwil
Copy link
Contributor

daviwil commented Nov 8, 2017

@rkeithhill I use Invoke-Build Test[Host|Server|Protocol]? to run them, the project build is a little more complex and is driven by the build script these days.

@daviwil
Copy link
Contributor

daviwil commented Nov 8, 2017

You may be right with this fix, actually. I've seen some instances recently where out of range exceptions get thrown when working with file positions, maybe this was it. Can you still get IntelliSense in the last line of a file with this fix?

@rkeithhill
Copy link
Contributor Author

That's better than what I've tried - installing xunit console runner from NuGet and invoking it directly. :-)

RE - Intellisense on the last line - I'll check that tonight.

@rkeithhill
Copy link
Contributor Author

OK, yeah auto-completion works on the last line, last column. BTW I see this in the VS output window while I'm debugging.

Dispatching event "$/cancelRequest"
Dispatching message "$/cancelRequest" HAS NO HANDLER TO AWAIT

Is that harmless or ...?

@rkeithhill
Copy link
Contributor Author

OK I think I fixed the issue the test found. Stupid mistake on my part. The builds appear to be hung. Is there a way you can kick or restart them?

@rkeithhill
Copy link
Contributor Author

@daviwil Can you kick the Travis build? It passed on Linux but failed to clone the repo on macOS. I think this PR is good to go but it would be nice to get a clean pass through the tests.

@daviwil
Copy link
Contributor

daviwil commented Nov 9, 2017

Just restarted the build!

@daviwil
Copy link
Contributor

daviwil commented Nov 9, 2017

Travis CI is super slow and unreliable today for macOS builds...

@daviwil
Copy link
Contributor

daviwil commented Nov 9, 2017

It's building fine on Linux, I'd say go ahead and merge it even though the macOS side isn't running (their fault).

@rkeithhill rkeithhill merged commit e19ab6f into PowerShell:master Nov 9, 2017
@rkeithhill rkeithhill deleted the rkeithhill/fix-validate-position branch November 9, 2017 20:12
TylerLeonhardt pushed a commit to TylerLeonhardt/PowerShellEditorServices that referenced this pull request Feb 26, 2019
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.

2 participants