Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Add an icon for the "Run Python File in Terminal" command. - #6498

#6498
Merged
ericsnowcurrently merged 21 commits into
microsoft:mastermicrosoft/vscode-python:masterfrom
ericsnowcurrently:run-in-terminal-iconericsnowcurrently/vscode-python:run-in-terminal-iconCopy head branch name to clipboard
Jul 29, 2019
Merged

Add an icon for the "Run Python File in Terminal" command.#6498
ericsnowcurrently merged 21 commits into
microsoft:mastermicrosoft/vscode-python:masterfrom
ericsnowcurrently:run-in-terminal-iconericsnowcurrently/vscode-python:run-in-terminal-iconCopy head branch name to clipboard

Conversation

@ericsnowcurrently

@ericsnowcurrently ericsnowcurrently commented Jul 8, 2019

Copy link
Copy Markdown

(for #5321)

Note that we will add the final icons once they are settled. Until then we use placeholders.

Also note that I called the experiment "ShowPlayIcon".

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
  • Title summarizes what is changing
  • Has a news entry file (remember to thank yourself!)
  • Appropriate comments and documentation strings in the code
  • Has sufficient logging.
  • Has telemetry for enhancements.
  • [ ] Unit tests & system/integration tests are added/updated
  • [ ] Test plan is updated as appropriate
  • [ ] package-lock.json has been regenerated by running npm install (if dependencies have changed)
  • [ ] The wiki is updated with any design decisions/details.

@karrtikr karrtikr left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Do we want to ship the experiments right now? If not, please remove changes to experiments.json.

Comment thread src/client/extension.ts Outdated
Comment thread src/client/terminals/activation.ts Outdated
@ericsnowcurrently

Copy link
Copy Markdown
Author

FYI, this is blocked on getting the final icons.

@ericsnowcurrently

Copy link
Copy Markdown
Author

@karrtikr, yeah, we're going to ship the experiment.

@karrtikr karrtikr left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Please use IExtensionActivationService, and add tests.

Comment thread src/client/terminals/serviceRegistry.ts
Comment thread src/client/terminals/codeExecution/codeExecutionManager.ts
Comment thread src/client/terminals/activation.ts
Comment thread src/client/terminals/activation.ts Outdated
Comment thread src/client/terminals/activation.ts Outdated
@ericsnowcurrently

Copy link
Copy Markdown
Author

So once I've added tests we're good to go?

@karrtikr karrtikr left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Add tests and this should be GTG.

Comment thread src/client/terminals/activation.ts Outdated
Comment thread src/client/terminals/activation.ts Outdated
Comment thread src/client/terminals/activation.ts Outdated
Comment thread src/client/terminals/activation.ts Outdated
Comment thread src/client/terminals/activation.ts Outdated
Comment thread src/test/terminals/serviceRegistry.unit.test.ts Outdated
Comment thread src/test/terminals/codeExecution/codeExecutionManager.unit.test.ts Outdated
Comment thread src/test/terminals/codeExecution/codeExecutionManager.unit.test.ts Outdated
Comment thread src/client/terminals/activation.ts Outdated
Comment thread src/client/terminals/activation.ts Outdated
@ericsnowcurrently
ericsnowcurrently merged commit 76f5fd8 into microsoft:master Jul 29, 2019
@ericsnowcurrently
ericsnowcurrently deleted the run-in-terminal-icon branch July 29, 2019 20:25
@lock lock Bot locked as resolved and limited conversation to collaborators Aug 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.