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

Read command #736

Merged
merged 11 commits into from
Sep 15, 2016
Merged

Read command #736

merged 11 commits into from
Sep 15, 2016

Conversation

domgee
Copy link
Contributor

@domgee domgee commented Sep 8, 2016

Initial support for read and read!.

Vim documentation, read
Vim documentation, read!

Range and opt arguments are not yet implemented.

@@ -233,6 +233,7 @@
"postinstall": "node ./node_modules/vscode/bin/install && gulp init"
},
"dependencies": {
"child-process": "^1.0.2",
Copy link
Member

Choose a reason for hiding this comment

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

Indent :)

@rebornix
Copy link
Member

rebornix commented Sep 9, 2016

Thanks for taking things that are currently not on our list :) Just drop some comments and can you update the roadmap in your PR as well, especially add Note to it if child-process is not OS-compatible (I'm not sure).

@domgee
Copy link
Contributor Author

domgee commented Sep 9, 2016

I've updated the roadmap and dropped one comment at least :)

}

async getTextToInsertFromFile() : Promise<string> {
// TODO: Substitute utf8 with current file encoding, couldn't find this anywhere in vscode's api.
Copy link
Member

Choose a reason for hiding this comment

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

Is there an issue tracking this with VSCode? Some initial Google says there's some config at the workspace level where you can set the encoding. Maybe we can read that config?

@jpoon
Copy link
Member

jpoon commented Sep 11, 2016

@dominic31 awesome work! dropped a few comments.

@domgee
Copy link
Contributor Author

domgee commented Sep 12, 2016

@jpoon thanks for the comments! I've left some replies.

@jpoon
Copy link
Member

jpoon commented Sep 12, 2016

LGTM.

  • If you choose not to include the detection of the current file encoding, can you file a separate issue so we can track it?
  • Update the test

@domgee
Copy link
Contributor Author

domgee commented Sep 13, 2016

I've updated the unit test.

About the encoding, there's an open issue on the VS Code project for supporting encoding related functions through the api. It hasn't got past the backlog yet. Shall I open an issue here that refers to that issue?

@jpoon
Copy link
Member

jpoon commented Sep 13, 2016

Yes please!

One last thing, can you merge with master and resolve the conflicts?

@domgee
Copy link
Contributor Author

domgee commented Sep 13, 2016

Sorry, I've gotten things a little muddled up here, the encoding parameter in fs.readFile() is of course the encoding of the external file that gets read in, not the current file in VS Code. Looking at the vim docs for read (link in the first comment), the encoding can be specified in the ++opt arguments that haven't yet been implemented. I'll update the comment in getTextToInsertFromFile().

Should I open an issue for implementing ++opt args, or just mention it in the roadmap?

@johnfn
Copy link
Member

johnfn commented Sep 15, 2016

I'm okay with this not having ++opt, that can be a separate task. :)

@johnfn johnfn merged commit c16a57c into VSCodeVim:master Sep 15, 2016
@domgee domgee deleted the read-command branch September 15, 2016 08:48
@domgee domgee restored the read-command branch September 15, 2016 08:49
@domgee domgee deleted the read-command branch September 15, 2016 08:50
This pull request was closed.
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.

4 participants