Skip to content

Minor changes to dispatchers (action creators will dispatch multiple actions)#9920

Merged
DonJayamanne merged 14 commits intomicrosoft:ds/custom_editorfrom
DonJayamanne:fixDispatchers
Feb 10, 2020
Merged

Minor changes to dispatchers (action creators will dispatch multiple actions)#9920
DonJayamanne merged 14 commits intomicrosoft:ds/custom_editorfrom
DonJayamanne:fixDispatchers

Conversation

@DonJayamanne
Copy link
Copy Markdown

@DonJayamanne DonJayamanne commented Feb 5, 2020 •
edited
Loading

For #9898
For #9102

  • More strong typing for actions created (found one scenario where we weren't passing in the right stuff)
  • Fixed two bugs related to adding cells and deleting all cells
  • Fixed a bug related to adding cells using a and b keys, old approach resulted in a and b being typed into the new cell that was created as a result
  • Re-used actions to add cells (makes it much easier for syncing of messages across)
  • Fixed a bug in react (we were expecting an HTML dom element, however I think we were getting an canvas element where className was not a string but an object.
  • Toggle between MD and code should not set focus to code.

@DonJayamanne DonJayamanne added the no-changelog No news entry required label Feb 5, 2020
rchiodo
rchiodo reviewed Feb 5, 2020
View reviewed changes
Comment thread src/client/datascience/interactive-ipynb/nativeEditor.ts Outdated
rchiodo
rchiodo reviewed Feb 5, 2020
View reviewed changes
Comment thread src/datascience-ui/native-editor/redux/actions.ts Outdated
rchiodo
rchiodo requested changes Feb 5, 2020
View reviewed changes
Copy link
Copy Markdown

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

🕐

@codecov-io
Copy link
Copy Markdown

codecov-io commented Feb 5, 2020 •
edited
Loading

Codecov Report

Merging #9920 into ds/custom_editor will increase coverage by 60.78%.
The diff coverage is n/a.

Impacted file tree graph

@@                  Coverage Diff                  @@
##           ds/custom_editor    #9920       +/-   ##
=====================================================
+ Coverage                 0%   60.78%   +60.78%     
=====================================================
  Files                    10      564      +554     
  Lines                    31    29882    +29851     
  Branches                  4     4492     +4488     
=====================================================
+ Hits                      0    18165    +18165     
- Misses                   31    10722    +10691     
- Partials                  0      995      +995
Impacted Files Coverage Δ
...ience/interactive-common/interactiveWindowTypes.ts 100% <ø> (ø)
src/client/language/textRangeCollection.ts 85.45% <0%> (ø)
.../application/diagnostics/commands/launchBrowser.ts 100% <0%> (ø)
...c/client/interpreter/autoSelection/rules/system.ts 100% <0%> (ø)
...er/locators/services/workspaceVirtualEnvService.ts 81.81% <0%> (ø)
...datascience/jupyter/liveshare/hostJupyterServer.ts 11% <0%> (ø)
src/client/common/application/types.ts 100% <0%> (ø)
src/client/terminals/codeExecution/repl.ts 100% <0%> (ø)
src/client/linters/flake8.ts 85.71% <0%> (ø)
src/client/testing/main.ts 14.87% <0%> (ø)
... and 551 more

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 909d469...3b4d1db. Read the comment docs.

@DonJayamanne
Misc
19b1934
@DonJayamanne
Address code review comments
e43d169
@DonJayamanne
Merge branch 'ds/custom_editor' into fixDispatchers
ec4bdb8 
* ds/custom_editor: (44 commits)
  Implement undo support for the custom editor (#9946)
  Change cursor style of buttons to pointer (#9989)
  Command to select a kernel for a Notebook (#9988)
  Make error lines match the original file (#9984)
  Display commands when necessary (#9986)
  add the ip argument of jupyter server for k8s container (#9977)
  When starting Jupyter activate conda env (#9972)
  Make the interactive window wrap like the native editor does (#9966)
  Disables the use of a terminal to activate an environment (#9967)
  Add tests (excluding smoke) to CI GitHub Action (#9924)
  fix kernel and server labels when not connected (#9965)
  Be more clear about the virtual environment activation instructions
  Update bug report template (#9957)
  Do not override kernels info in other notebooks (#9958)
  Update instructions for creating the release branch
  Notebook should use kernel from metadata (#9936)
  Disable the terminal activation experiment (#9937)
  Fix variable explorer when restarting a kernel (#9931)
  Use the autoStart server if it matches when starting a new server (#9929)
  Capitalize Activate.ps1 (#9911)
  ...
@DonJayamanne
Oops
5b9b0d6
@DonJayamanne
More oops
b27a966
@DonJayamanne DonJayamanne requested a review from rchiodo February 7, 2020 23:48
rchiodo
rchiodo reviewed Feb 8, 2020
View reviewed changes
Comment thread src/client/datascience/interactive-ipynb/nativeEditor.ts Outdated
rchiodo
rchiodo approved these changes Feb 8, 2020
View reviewed changes
Copy link
Copy Markdown

@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:

rchiodo
rchiodo reviewed Feb 8, 2020
View reviewed changes
Comment thread src/client/datascience/interactive-common/interactiveWindowTypes.ts Outdated
public [InteractiveWindowMessages.NotebookRunAllCells]: never | undefined;
public [InteractiveWindowMessages.NotebookRunSelectedCell]: never | undefined;
public [InteractiveWindowMessages.NotebookAddCellBelow]: IAddCellAction;
public [CommonActionType.FOCUS_CELL]: ICellAndCursorAction;
Copy link
Copy Markdown

@rchiodo rchiodo Feb 8, 2020

Choose a reason for hiding this comment

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

You shouldn't need this anymore, right? It's not an interactive window message

Copy link
Copy Markdown
Author

@DonJayamanne DonJayamanne Feb 8, 2020

Choose a reason for hiding this comment

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

Still used.

Copy link
Copy Markdown
Author

@DonJayamanne DonJayamanne Feb 8, 2020

Choose a reason for hiding this comment

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

Its not an interactive window message, but used in the mapping.

Copy link
Copy Markdown

@rchiodo rchiodo Feb 8, 2020

Choose a reason for hiding this comment

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

Isn't there another mapping for common action types? Seems like this is polluting the interactive window side stuff.

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

Copy link
Copy Markdown
Author

@DonJayamanne DonJayamanne Feb 8, 2020

Choose a reason for hiding this comment

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

Ok, I'll move it out in a separate PR.

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Feb 9, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@DonJayamanne DonJayamanne merged commit 97ad678 into microsoft:ds/custom_editor Feb 10, 2020
@DonJayamanne DonJayamanne deleted the fixDispatchers branch February 10, 2020 01:27
@lock lock Bot locked as resolved and limited conversation to collaborators Feb 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@rchiodo rchiodo rchiodo approved these changes

@DavidKutu DavidKutu Awaiting requested review from DavidKutu

@IanMatthewHuff IanMatthewHuff Awaiting requested review from IanMatthewHuff

Assignees

No one assigned

Labels

no-changelog No news entry required

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@DonJayamanne @codecov-io @rchiodo