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

Add ability to open untitled document with initial content #22021

Merged
merged 2 commits into from Mar 7, 2017

Conversation

Projects
None yet
5 participants
@hoovercj
Member

hoovercj commented Mar 5, 2017

Addresses #21413 by passing file contents down to the UntitledEditorModel.

I checked that it worked locally and I added a test case, but I've had issues getting tests to run as reported in #22019.

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Mar 5, 2017

@hoovercj, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bpasero and @alexandrudima to be potential reviewers.

mention-bot commented Mar 5, 2017

@hoovercj, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bpasero and @alexandrudima to be potential reviewers.

@msftclas

This comment has been minimized.

Show comment
Hide comment
@msftclas

msftclas Mar 5, 2017

@hoovercj,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

msftclas commented Mar 5, 2017

@hoovercj,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

@bpasero bpasero self-assigned this Mar 5, 2017

@bpasero bpasero added this to the March 2017 milestone Mar 7, 2017

@bpasero bpasero merged commit 503f8f5 into Microsoft:master Mar 7, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@bpasero

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero Mar 7, 2017

Member

@hoovercj thanks.

/cc @jrieken for the API change

Member

bpasero commented Mar 7, 2017

@hoovercj thanks.

/cc @jrieken for the API change

@hoovercj

This comment has been minimized.

Show comment
Hide comment
@hoovercj

hoovercj Mar 7, 2017

Member

Can you elaborate on when you use a leading underscore and when you don't?

Member

hoovercj commented on ceba929 Mar 7, 2017

Can you elaborate on when you use a leading underscore and when you don't?

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero Mar 7, 2017

Member

@hoovercj I never use leading underscores unless the class has a get property defined (e.g. get foo(): any and then _foo for the actual value).

Member

bpasero replied Mar 7, 2017

@hoovercj I never use leading underscores unless the class has a get property defined (e.g. get foo(): any and then _foo for the actual value).

@@ -100,6 +100,15 @@ suite('workspace-namespace', () => {
});
});
test('openTextDocument, untitled without path but language ID and contents', function () {
return workspace.openTextDocument({ language: 'html', contents: '<h1>Hello world!</h1>' }).then(doc => {

This comment has been minimized.

@jrieken

jrieken Mar 7, 2017

Member

singular, content?

@jrieken

jrieken Mar 7, 2017

Member

singular, content?

@@ -4085,7 +4085,7 @@ declare module 'vscode' {
* @param options Options to control how the document will be created.
* @return A promise that resolves to a [document](#TextDocument).
*/
export function openTextDocument(options?: { language: string; }): Thenable<TextDocument>;
export function openTextDocument(options?: { language?: string; contents?: string; }): Thenable<TextDocument>;

This comment has been minimized.

@jrieken

jrieken Mar 7, 2017

Member

missing jsdoc-comment, also use content to align with TextDocumentContentProvider

@jrieken

jrieken Mar 7, 2017

Member

missing jsdoc-comment, also use content to align with TextDocumentContentProvider

@jrieken

This comment has been minimized.

Show comment
Hide comment
@jrieken

jrieken Mar 7, 2017

Member

re #22021 (comment) - that's a personal preference ;-) For me, private fields are underscore-prefixed because that helps me read method bodies. Others use underscore only if a property would collide with a getter/setter otherwise.

Member

jrieken commented Mar 7, 2017

re #22021 (comment) - that's a personal preference ;-) For me, private fields are underscore-prefixed because that helps me read method bodies. Others use underscore only if a property would collide with a getter/setter otherwise.

@bpasero

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero Mar 7, 2017

Member

As a rule of thumb, when adding to existing code, we should not start a new style but apply the style the code was originally written in.

@hoovercj since I already merged this change, I suggest a new PR just for the 💄 changes.

Member

bpasero commented Mar 7, 2017

As a rule of thumb, when adding to existing code, we should not start a new style but apply the style the code was originally written in.

@hoovercj since I already merged this change, I suggest a new PR just for the 💄 changes.

@hoovercj

This comment has been minimized.

Show comment
Hide comment
@hoovercj

hoovercj Mar 7, 2017

Member

@bpasero I agree with the consistent style comment. I was confused by _hasAssociatedFilePath which does have the leading _. To me the initial value seems closer to _hasAssociatedFilePath than modeId so I also used a leading _.

Regarding a new PR, you are suggesting that I make a new PR that changes contents to content, adds a jsdoc comment, etc.?

I can hopefully get to that tonight.

Member

hoovercj commented Mar 7, 2017

@bpasero I agree with the consistent style comment. I was confused by _hasAssociatedFilePath which does have the leading _. To me the initial value seems closer to _hasAssociatedFilePath than modeId so I also used a leading _.

Regarding a new PR, you are suggesting that I make a new PR that changes contents to content, adds a jsdoc comment, etc.?

I can hopefully get to that tonight.

@bpasero

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero Mar 7, 2017

Member

@hoovercj _hasAssociatedFilePath makes sense because of the getter for it.

Member

bpasero commented Mar 7, 2017

@hoovercj _hasAssociatedFilePath makes sense because of the getter for it.

@hoovercj

This comment has been minimized.

Show comment
Hide comment
@hoovercj

hoovercj Mar 7, 2017

Member

Yep, now I see that, and that modeId doesn't have a getter, it has getModeId(). No name conflict so no _. Thanks for the clarification

Member

hoovercj commented Mar 7, 2017

Yep, now I see that, and that modeId doesn't have a getter, it has getModeId(). No name conflict so no _. Thanks for the clarification

hoovercj pushed a commit to hoovercj/vscode that referenced this pull request Mar 7, 2017

hoovercj pushed a commit to hoovercj/vscode that referenced this pull request Mar 7, 2017

bpasero added a commit that referenced this pull request Mar 8, 2017

Merge pull request #22178 from hoovercj/openWithContentLipstick
Follow up to PR #22021 to respond to feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment