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

Support mapping cells to hash/execution counts #6338

Merged
merged 12 commits into from
Jun 27, 2019

Conversation

rchiodo
Copy link

@rchiodo rchiodo commented Jun 25, 2019

For #6318

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

@rchiodo rchiodo self-assigned this Jun 25, 2019
break;

case InteractiveWindowMessages.RestartKernel:
this.hashes.clear();
Copy link
Member

@IanMatthewHuff IanMatthewHuff Jun 27, 2019

Choose a reason for hiding this comment

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

Is this just to be safe? We add sys into on a restart so that should already be clearing this. #Resolved

Copy link
Author

Choose a reason for hiding this comment

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

True. I had just this originally and then added the sysinfo ones. Might be better to remove it as future people would wonder why it's necessary.


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

}

public getHashes(): IFileHashes[] {
return [...this.hashes.entries()].map(e => {
Copy link
Member

@IanMatthewHuff IanMatthewHuff Jun 27, 2019

Choose a reason for hiding this comment

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

e [](start = 46, length = 1)

I think changing this to ([key, value]) makes the rest of the code clearer as you can skip the [0] and [1] s and just use key and value. Or change value to hashArray or something like that. #WontFix

Copy link
Author

Choose a reason for hiding this comment

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

Good idea


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

Copy link
Author

Choose a reason for hiding this comment

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

Not possible in typescript :(


In reply to: 298254468 [](ancestors = 298254468,298250981)

private onAboutToAddCode(args: IRemoteAddCode) {
// Make sure this is valid
if (args && args.code && args.line !== undefined && args.file) {
// First make sure not a markdown cell. Those can be ignored. Just get out the first code cell
Copy link
Member

@IanMatthewHuff IanMatthewHuff Jun 27, 2019

Choose a reason for hiding this comment

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

Just get out the first code cell [](start = 74, length = 32)

Do we ever add multiple code cells in a call of this? If so do we just ignore the next code cells in the call? #ByDesign

Copy link
Author

Choose a reason for hiding this comment

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

No this is just the case where the user has a markdown cell with code in it. There will only ever be at most a single cell.


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

Copy link
Author

Choose a reason for hiding this comment

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

Maybe I should put a comment to that effect though.


In reply to: 298254091 [](ancestors = 298254091,298252462)

Copy link
Author

Choose a reason for hiding this comment

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

Actually no that's wrong. RunSelectionOrLine could theoretically span multiple cells.


In reply to: 298254188 [](ancestors = 298254188,298254091,298252462)

Copy link
Author

Choose a reason for hiding this comment

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

Ah actually what that does is send the all the cells in one big chunk. Makes sense. The user said to run the entire selection as one cell.


In reply to: 298255279 [](ancestors = 298255279,298254188,298254091,298252462)

Copy link
Member

Choose a reason for hiding this comment

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

Do the run entire file commands do the same thing? That was the one that I was initially thinking of.


In reply to: 298261612 [](ancestors = 298261612,298255279,298254188,298254091,298252462)

Copy link
Author

Choose a reason for hiding this comment

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

Yeah they either split the file into cells or send the entire thing in one chunk. This would still only generate at most a single code cell.


In reply to: 298262026 [](ancestors = 298262026,298261612,298255279,298254188,298254091,298252462)

Copy link
Member

@IanMatthewHuff IanMatthewHuff 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 merged commit 9002be4 into master Jun 27, 2019
@rchiodo rchiodo deleted the rchiodo/debugger_codemapping branch June 27, 2019 16:43
@lock lock bot locked as resolved and limited conversation to collaborators Jul 29, 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.

2 participants