-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Refactor the existing file opening and auto completion #4032
Refactor the existing file opening and auto completion #4032
Conversation
This is to allow command with extra spaces in front and back to run
Travis tests have failedHey @stevenguh, Node.js: 8if [[ $(git diff-index HEAD -- *.js *.ts *.md) ]]; then git diff; echo "Prettier Failed. Run `gulp forceprettier` and commit changes to resolve."; exit 1; fi
npm test
TravisBuddy Request Identifier: 6481c1b0-cd94-11e9-984e-111da162f93c |
This commit refactors the existing auto complete mechanism to be more consistent with the actual Vim while adding support for VS Code Remote. Fixes VSCodeVim#3815
In the actual Vim, commands like ":e" do not create a file first if the path in the cmd doesn't exist. Rather, it creates an empty buffer and only create the file on write. This commits changes the behavior to be more consistent with actual Vim by using untitled document instead creating the file if the file on the cmd is not found.
8f0c077
to
f648331
Compare
Travis tests have failedHey @stevenguh, Node.js: 8if [[ $(git diff-index HEAD -- *.js *.ts *.md) ]]; then git diff; echo "Prettier Failed. Run `gulp forceprettier` and commit changes to resolve."; exit 1; fi
npm test
TravisBuddy Request Identifier: 39303580-cd96-11e9-984e-111da162f93c |
src/util/path.ts
Outdated
} | ||
const directoryResult = await vscode.workspace.fs.readDirectory(directoryUri); | ||
return directoryResult | ||
.map(d => ({ path: d[0] + (d[1] === vscode.FileType.Directory ? sep : ''), type: d[1] })) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't figure out what's going on here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
f[0] is the item name in a directory, and d[1] is the type of the item.
Basically, I am appending separator at the end as the indicator of directory item.
I refactored this method a bit so it only returns an array of strings.
statusBarAfterTab = StatusBar.Get().trim(); | ||
assert.equal(statusBarAfterTab, ':edit|it', 'Failed to complete content left of the cursor'); | ||
|
||
await modeHandler.handleKeyEvent('<Esc>'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validate that we've exited tab completion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we need to, and how.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's remove line 162 then ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't have at the end, it will mess up other tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests shouldn't be dependent on each other. If this test fails causing other test failures, it will be difficult to deduce the cause. Can we add a clean-up method following this test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. How about I call always after a test case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your PR! Sorry I reviewed this in phases; took a break for dinner in-between.
Let us know when the PR is ready for review again. |
Oh..it's ready. |
Travis tests have failedHey @stevenguh, Node.js: 8if [[ $(git diff-index HEAD -- *.js *.ts *.md) ]]; then git diff; echo "Prettier Failed. Run `gulp forceprettier` and commit changes to resolve."; exit 1; fi
npm test
TravisBuddy Request Identifier: cb7fbe20-d7e5-11e9-b57f-f7c3bbd28520 |
What this PR does / why we need it:
The current implementation of file completion and file opening has way too many cases that don't work, and behavior that are not consistent with Vim. To name a few:
This pull request contains commits that refactor the existing auto complete mechanism to be more consistent with Vim while adding support for VS Code Remote.
This is the list of the expected behaviors:
Concrete examples: (The ":e" command can be any file commands like ":vsp")
Windows Only behaviors:
Which issue(s) this PR fixes
Fixes #3815
Fixes #3824
Special notes for your reviewer:
In the future, we can resolve the current directory of an untitled tab with workspace. However, this is complicated by the fact that multi-folder workspace is possible. But I chose to not do that, because it would complicate this pull requests.
Since this pull request combined a few features and refactor. It may be easier to view the changes commit by commit.