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.

Sign up for GitHub

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

Jump to bottom

fix: #55946 Added comment snippet variable #63572

Merged
merged 4 commits into from Nov 22, 2018
Merged

fix: #55946 Added comment snippet variable #63572

merged 4 commits into from Nov 22, 2018

Conversation

karanisverma
Copy link
Contributor

@karanisverma karanisverma commented Nov 21, 2018 •
edited

Added comment snippet variable
it fixes following issue
#55946

@msftclas
Copy link

msftclas commented Nov 21, 2018 •
edited

CLA assistant check
All CLA requirements met.

jrieken
jrieken reviewed Nov 21, 2018
View reviewed changes
src/vs/editor/contrib/snippet/snippetVariables.ts Outdated
} else if (name === 'BLOCK_COMMENT_START') {
return comments.blockCommentStartToken;
} else if (name === 'BLOCK_COMMENT_END') {
return comments.blockCommentEndToken;
Copy link
Member

@jrieken jrieken Nov 21, 2018 •
edited

Choose a reason for hiding this comment

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

... || undefined to ensure the default value is applied (in all cases)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I have made changes as per your suggestion.

@jrieken jrieken self-assigned this Nov 21, 2018
@jrieken jrieken added this to the November 2018 milestone Nov 21, 2018
@jrieken
Copy link
Member

jrieken commented Nov 21, 2018 •
edited

@karanisverma The build has failed due to a strict null error. You can them locally via F1 > Run Task > Strict Null Checks - it will then squiggle the code for null-violations

@karanisverma
Copy link
Contributor Author

Thanks for the info, Looking into it :)

@karanisverma
Copy link
Contributor Author

@jrieken I have fixed the issue and I have checked locally by following the steps you mentioned above.
Sharing log for test below:

> Executing task: npm run strict-null-check <


> code-oss-dev@1.30.0 strict-null-check /Users/karanverma/me/oss/vscode
> tsc -p src/tsconfig.strictNullChecks.json


Terminal will be reused by tasks, press any key to close it.

build process seems to fail in following file
src/vs/base/browser/ui/tree/abstractTree.ts(205,12): error TS2532: Object is possibly 'undefined'.

I guess it's not because of the changes in this PR. Please let me know if there is anything I have to fix at my end.

@karanisverma
Copy link
Contributor Author

@jrieken I have pulled code from master, tests are passing now 🙌🏼

jrieken
jrieken approved these changes Nov 22, 2018
View reviewed changes
Copy link
Member

@jrieken jrieken left a comment

Choose a reason for hiding this comment

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

lgtm

@jrieken jrieken merged commit f325118 into microsoft:master Nov 22, 2018
@jrieken
Copy link
Member

jrieken commented Nov 22, 2018

Thanks @karanisverma

@karanisverma
Copy link
Contributor Author

Anytime :), Thanks you for guiding me 🙂

@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jrieken jrieken jrieken approved these changes

Assignees

@jrieken jrieken

Labels
None yet
Projects
None yet
Milestone
November 2018
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@karanisverma @msftclas @jrieken