Skip to content

Conversation

IanMatthewHuff
Copy link
Member

For #7800

  • 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.

this.submitInput(concatMultilineString(selectedCell.cell.data.source), selectedCell);
}
}
this.resumeUpdates();
Copy link

@rchiodo rchiodo Oct 7, 2019

Choose a reason for hiding this comment

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

resumeUpdates [](start = 13, length = 13)

I don't think we need this around submission of input. This is really just suspend state updates. Doesn't seem necessary here. #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

Would it not be needed in the existing runAll code? Or do we need it there because we might be doing it for multiple cells?


In reply to: 332283022 [](ancestors = 332283022)

Copy link

Choose a reason for hiding this comment

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

Yeah it's for multiple cells. It basically postpones any state updates until resumeUpdates get called, meaning no renders happen. It doesn't really hurt here though. You could just leave it to be consistent.


In reply to: 332284555 [](ancestors = 332284555,332283022)

Copy link
Member Author

Choose a reason for hiding this comment

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

Nah, if it's not needed it's not needed. I was just copying the "only one" case from run all. I'll remove it.


In reply to: 332285496 [](ancestors = 332285496,332284555,332283022)

"when": "python.datascience.havenative && python.datascience.featureenabled"
},
{
"command": "python.datascience.notebookeditor.runselectedcell",
Copy link

@rchiodo rchiodo Oct 7, 2019

Choose a reason for hiding this comment

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

runselectedcell [](start = 66, length = 15)

Does this need a check to see if there is a selected cell? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it doesn't really do anything without that. I'll add it into the send info and have a context for it.


In reply to: 332283220 [](ancestors = 332283220)

Copy link

@rchiodo rchiodo left a comment

Choose a reason for hiding this comment

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

:shipit:

@codecov-io
Copy link

codecov-io commented Oct 8, 2019

Codecov Report

Merging #7807 into master will decrease coverage by <.01%.
The diff coverage is 70%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7807      +/-   ##
==========================================
- Coverage   59.16%   59.16%   -0.01%     
==========================================
  Files         498      498              
  Lines       22254    22266      +12     
  Branches     3578     3580       +2     
==========================================
+ Hits        13166    13173       +7     
- Misses       8264     8269       +5     
  Partials      824      824
Impacted Files Coverage Δ
src/client/datascience/types.ts 100% <ø> (ø) ⬆️
...atascience/interactive-window/interactiveWindow.ts 23.91% <0%> (-0.81%) ⬇️
src/client/datascience/constants.ts 100% <100%> (ø) ⬆️
...ience/interactive-common/interactiveWindowTypes.ts 100% <100%> (ø) ⬆️
.../datascience/interactive-common/interactiveBase.ts 6.22% <0%> (-0.03%) ⬇️
src/client/datascience/jupyter/jupyterExecution.ts 52.53% <0%> (ø) ⬆️
...rc/client/datascience/errorHandler/errorHandler.ts 67.64% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1536d8b...a8f7501. Read the comment docs.

@IanMatthewHuff IanMatthewHuff merged commit 45f744d into microsoft:master Oct 8, 2019
@IanMatthewHuff IanMatthewHuff deleted the dev/ianhu/nativeCommandPalette branch October 8, 2019 16:49
@lock lock bot locked as resolved and limited conversation to collaborators Oct 15, 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.

3 participants