Added support for multiple selections in Open Link action#41242
Merged
jrieken merged 3 commits intomicrosoft:masterfrom Jan 8, 2018
Merged
Added support for multiple selections in Open Link action#41242jrieken merged 3 commits intomicrosoft:masterfrom
jrieken merged 3 commits intomicrosoft:masterfrom
Conversation
Member
|
@mlewand Changes look good but it seems that it needs to be formatted: https://travis-ci.org/Microsoft/vscode/jobs/326035552#L393. Please run 'Format Document' and push those changes. Thanks |
Contributor
Author
|
@jrieken oh, sorry that's embarrassing. I have pushed reformatted branch. Feel free to squash. Any hints on unit tests? I'd feel better if that stuff could be covered with unit tests. We could extract unit tests to a followup PR though. |
Member
|
Thanks, passes the hygiene tests now. Wrt tests I'd say we are OK because the changes aren't risky IMO. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #41231.
It checks for links in each selection, rather than limiting to a single one.
Unit Tests
tl;dr Unfortunately I was unable to integrate all deps to make tests together.
I'm a fan of TDD, however I had to gave up during unit tests development. As there were many dependencies going on there.
Based on other tests I was able to somehow register
LinkDetectorcontribution to the editor, but that wasn't enough, as it relies oncurrentOccurrenceswhich is set in updateDecorations and I wasn't able to figure out api to hook it up to the editor. I guess I should try to hookLinkProviderRegistrybut I wasn't able to.Then as a last resort I wanted to stub these dependencies using Sinon, but again, even stubbed methods had quite a few dependencies (
getLinkOccurrencemethod), which would require me to manually create ILink compatible instances - and I don't think this is the best idea.All the work is available at my
open-link-multi-selections-testsbranch. Feel free to pull it if you find it helpful to do final touches, or you could point me to how to make it work together.