Skip to content

Make gather an optional component#9755

Merged
greazer merged 78 commits into
masterfrom
dev/jim/use-new-gather
Mar 8, 2020
Merged

Make gather an optional component#9755
greazer merged 78 commits into
masterfrom
dev/jim/use-new-gather

Conversation

@greazer
Copy link
Copy Markdown
Member

@greazer greazer commented Jan 24, 2020

This PR does two things. First, it modifies the building of gather functionality to make use of a non OSS component. Second, it adjusts the existing CellHash management to be per-notebook rather than a singleton. Both of the above were related, so it made sense to do it in one PR.

  • 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. Coming in subsequent PR.
  • 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.

cell executionId for gather.
Dynamically look for python-program-analysis. If not there
gather is unsupported.
Comment thread src/client/datascience/jupyter/liveshare/hostJupyterNotebook.ts Outdated
Comment thread src/client/datascience/jupyter/jupyterNotebook.ts Outdated
Comment thread src/client/datascience/jupyter/jupyterNotebook.ts Outdated
Comment thread src/test/datascience/dataScienceIocContainer.ts Outdated
Comment thread src/test/datascience/interactiveWindowTestHelpers.tsx Outdated
Comment thread src/test/datascience/mockJupyterNotebook.ts
Comment thread types/@msrvida-python-program-analysis/genspec/main.d.ts
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.

🕐

Comment thread .prettierrc.js
Copy link
Copy Markdown

@DonJayamanne DonJayamanne left a comment

Choose a reason for hiding this comment

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

@rchiodo @greazer
I don't know much about gather now how it works/what it does.

Looking at the code, the gather needs to be tied to a specific notebook, and we do this by using some hops by using IInteractiveWindowListener, then going through all listeners and finding the specific item and casting to a type, then getting something out of that (service property, etc). This seems quite complicated to me.

Shouldn't we have a better way to tie these instances to a specific notebook.
I.e. either each class maintains its own collection of objects against notebooks (its own map/dict). Then ensure we pass the notebook uri (Interactive Window/Native Editor) objects to some of these interfaces, such as ICellHashListener and ICellHashProvider.

Comment thread src/client/datascience/editor-integration/cellhashLogger.ts
Comment thread src/client/datascience/editor-integration/cellhashprovider.ts Outdated
Comment thread src/datascience-ui/interactive-common/redux/reducers/helpers.ts
Comment thread src/datascience-ui/native-editor/nativeCell.tsx
Comment thread src/client/datascience/types.ts Outdated
Comment thread src/client/datascience/types.ts Outdated
Comment thread src/client/datascience/editor-integration/codeLensFactory.ts
Comment thread src/client/datascience/gather/gather.ts Outdated
Comment thread src/client/datascience/gather/gatherLogger.ts
Comment thread src/client/datascience/types.ts Outdated
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:

Comment thread src/client/datascience/editor-integration/codeLensFactory.ts
Comment thread src/client/datascience/gather/gatherListener.ts Outdated
Comment thread src/client/datascience/gather/gatherListener.ts Outdated
Comment thread src/client/datascience/jupyter/jupyterNotebook.ts
Comment thread src/client/datascience/editor-integration/codeLensFactory.ts
Copy link
Copy Markdown

@DonJayamanne DonJayamanne left a comment

Choose a reason for hiding this comment

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

.

Comment thread src/client/datascience/gather/gatherListener.ts
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Mar 8, 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
0.0% 0.0% Duplication

@greazer greazer merged commit d1b2204 into master Mar 8, 2020
@lock lock Bot locked as resolved and limited conversation to collaborators Mar 17, 2020
@DonJayamanne DonJayamanne deleted the dev/jim/use-new-gather branch June 19, 2020 17:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

no-changelog No news entry required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants