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.

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

Hide jupyter commands from other types of notebooks #5574

Merged
merged 3 commits into from
Apr 21, 2021

Conversation

DavidKutu
Copy link

For #5559

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

@DavidKutu DavidKutu requested a review from a team as a code owner April 20, 2021 22:10
Copy link
Contributor

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

I think we need a when clause instead

@rchiodo
Copy link
Contributor

rchiodo commented Apr 20, 2021

I believe we use 'Notebook' on purpose. Isn't this going to make all of those commands prefixed with 'Jupyter' instead? That seems more confusing to me.

@DavidKutu
Copy link
Author

@rchiodo I could leave only the changes to the when clause, that's enough to fix the issue.
As of the name Jupyter or Notebook, both are going to be confusing to a group of people. I don't mind leaving one or the other.

@DavidKutu DavidKutu requested a review from rchiodo April 20, 2021 23:21
@joyceerhl
Copy link
Contributor

joyceerhl commented Apr 21, 2021

@rchiodo I believe Matt said in #5559 that our using 'Notebook' is confusing, because the category also indicates where the command is coming from. E.g. 'Notebook' means 'VS Code-contributed' while 'Jupyter' means 'Jupyter extension-contributed'. Since this command is contributed by the Jupyter extension I think it makes sense to prefix it with 'Jupyter'. I'm also wondering if Peng/Matt have further thoughts on this.

@codecov-commenter
Copy link

codecov-commenter commented Apr 21, 2021

Codecov Report

Merging #5574 (eecc524) into main (689a2de) will decrease coverage by 0%.
The diff coverage is n/a.

@@          Coverage Diff          @@
##            main   #5574   +/-   ##
=====================================
- Coverage     72%     72%   -1%     
=====================================
  Files        402     402           
  Lines      26641   26641           
  Branches    3887    3887           
=====================================
- Hits       19444   19406   -38     
- Misses      5589    5619   +30     
- Partials    1608    1616    +8     
Impacted Files Coverage Δ
src/client/api/pythonApi.ts 64% <0%> (-11%) ⬇️
src/client/datascience/openNotebookBanner.ts 80% <0%> (-8%) ⬇️
...ience/notebook/emptyNotebookCellLanguageService.ts 72% <0%> (-7%) ⬇️
src/client/common/application/applicationShell.ts 23% <0%> (-5%) ⬇️
...rc/client/datascience/jupyter/debuggerVariables.ts 75% <0%> (-4%) ⬇️
src/client/datascience/raw-kernel/rawSocket.ts 84% <0%> (-2%) ⬇️
src/client/datascience/notebook/kernelProvider.ts 87% <0%> (-2%) ⬇️
src/client/datascience/telemetry/telemetry.ts 81% <0%> (-2%) ⬇️
...t/datascience/kernel-launcher/localKernelFinder.ts 81% <0%> (-1%) ⬇️
src/client/datascience/notebook/helpers/helpers.ts 72% <0%> (-1%) ⬇️
... and 5 more

@DonJayamanne
Copy link
Contributor

@joyceerhl I don't think we should change the command group to Jupyter.
I have replied her #5559 (comment)

It's something we discussed as a team (I think we even have an issue for this), I believe it came out of a QP study

@DavidKutu
Copy link
Author

@joyceerhl that's a good topic to bring up in our next sync meeting. I'd vote to have everything we contribute in the Jupyter group.

@DavidKutu DavidKutu merged commit ec0359b into main Apr 21, 2021
@DavidKutu DavidKutu deleted the david/specifyCommandWhen branch April 21, 2021 03:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants