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

Smile command #4070

Merged
merged 5 commits into from Sep 18, 2019
Merged

Smile command #4070

merged 5 commits into from Sep 18, 2019

Conversation

caleywoods
Copy link
Contributor

What this PR does / why we need it:
The VSCode Vim emulator does not replicate the :smile command from vim 8+; this simple addition and PR rectifies that. It's not mentioned on the roadmap but I had three reasons for this:

  • I have never messed with a VSCode extension
  • I love vim
  • This seemed very approachable (completed within an hour)

To use this command, with a file open in the editor, simply enter :smile and you should be greeted with a new untitled file which contains the ascii smile text seen when you enter the :smile command inside of vim.

Special notes for your reviewer:

Thank you for having things well structured and a descriptive contribution document. This may be unwanted or unneeded, if that's the case feel free to toss it aside, it was just an exercise in doing something I wanted to do.

@caleywoods
Copy link
Contributor Author

I realized after reading the Travis report I didn't add any tests for this, let me know if it's wanted and I can throw it into test/cmd_line to verify that a new file is opened and contains the desired text.

await TextEditor.insert(this.getSmileText());
}

getSmileText(): string {
Copy link
Member

@jpoon jpoon Sep 13, 2019

Choose a reason for hiding this comment

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

make this static readonly instead of a function

@jpoon
Copy link
Member

jpoon commented Sep 13, 2019

Interesting, I had no idea vim had such a command. Yes, please add tests!

@jpoon
Copy link
Member

jpoon commented Sep 13, 2019

Let us know when the PR is ready for review again.

@caleywoods
Copy link
Contributor Author

caleywoods commented Sep 16, 2019

Running Windows 10 x64

Getting a test suite failure on any suite that implements suiteSetup/setup or suiteTeardown/teardown when trying npm test. When trying to run within a docker container I receive: Error: spawn /app/.vscode-test/vscode-1.38.1/VSCode-linux-x64/code ENOENT Tests exited with code: -2. The tests that do this complain about: TypeError: Cannot read property 'subscriptions' of undefined and about "before all" or "before each" hooks. From what I gathered during looking for solutions it's something with the async/await style of the mocha tests, I found something talking about upping the mocha timeout but it looks like the timeout configured is already quite high and well beyond how quickly the errors will show up. I'm sure it's just my machine/environment, Windows always has to be difficult.

It mentions further logging available at /root/.npm/_logs/{timestamp}-debug.log but I'm not sure how to retrieve that since attempting to restart the docker container results in the same exit code and I'm not sure where that location might be on the host system if it exists.

Looks like any suite that just tests raw code functionality works correctly (via npm test).

Edit: I edited the Dockerfile to remove the entrypoint so that I could start the container without it dying automatically. Here is an example log that occurs within the container when you try to run npm run test: https://i.imgur.com/gp6B03O.png

Edit2: Here's the output when manually running the test script from within the node environment inside the container where it errors when trying to spawn the child process: https://gist.github.com/caleywoods/840dfcee6d859218044c519edeb79247 -- Upon checking the directory /app/.vscode-test/vscode-1.38.1 it appears to be the Windows version which would explain the failure.

@caleywoods
Copy link
Contributor Author

caleywoods commented Sep 16, 2019

Update: I have the docker test suite running. The fix from my above comment is to delete .vscode-test directory prior to executing gulp test if you've previously run npm test on Windows because that will have downloaded VSCode for windows. The reverse is also true, if you ran gulp test first and it downloaded VSCode for the Linux container, it will need to be removed so that npm test can download a Windows-compatible version.

Perhaps I should submit a PR which checks the host OS and if windows is detected and Code.exe can be found in the test directory it prompts or notifies the user of an impending issue requiring their action? Issue opened against vscode-test, a bandaid could be made to vscode-extension-vscode.

@caleywoods
Copy link
Contributor Author

@jpoon This is again ready for review. Sorry for the delay, I ventured down a test related rabbit hole because of Windows.

@jpoon jpoon merged commit a3f491a into VSCodeVim:master Sep 18, 2019
@jpoon
Copy link
Member

jpoon commented Sep 18, 2019

Thanks!

@caleywoods caleywoods deleted the smile-command branch October 5, 2019 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants