Skip to content

Add telemetry for imports#4736

Merged
rchiodo merged 5 commits into
masterfrom
rchiodo/import_telemetry
Mar 13, 2019
Merged

Add telemetry for imports#4736
rchiodo merged 5 commits into
masterfrom
rchiodo/import_telemetry

Conversation

@rchiodo

@rchiodo rchiodo commented Mar 13, 2019

Copy link
Copy Markdown

For #4718

  • 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!)
  • 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 Mar 13, 2019
});

function emitDocEvent(code: string, ev: EventEmitter<TextDocument>) {
const textDoc = createDocument(code, 'foo.py', 1, TypeMoq.Times.atMost(100), true);

@IanMatthewHuff IanMatthewHuff Mar 13, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

TypeMoq.Times.atMost(100) [](start = 58, length = 25)

The 100 is not actually meaningful here is it? There is a Times.max that would express "any amount is ok" if that was the intent here. #ByDesign

@rchiodo rchiodo Mar 13, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

That's what I wanted. Didn't find it. #ByDesign

@rchiodo rchiodo Mar 13, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Intellisense says otherwise? Is it liternally Times.max? #ByDesign

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks like I was reading the type wrong. The max and min are internal implementation details. I honestly don't see a way to express "any number of times" which is a bit funky but the best we can do I guess.


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

@codecov

codecov Bot commented Mar 13, 2019

Copy link
Copy Markdown

Codecov Report

Merging #4736 into master will decrease coverage by 1%.
The diff coverage is 89%.

@@           Coverage Diff           @@
##           master   #4736    +/-   ##
=======================================
- Coverage      77%     77%   -<1%     
=======================================
  Files         447     449     +2     
  Lines       21472   21978   +506     
  Branches     3527    3586    +59     
=======================================
+ Hits        16444   16792   +348     
- Misses       5024    5181   +157     
- Partials        4       5     +1
Flag Coverage Δ
#Linux 66% <78%> (+1%) ⬆️
#Windows 66% <78%> (+1%) ⬆️
#macOS 66% <78%> (+1%) ⬆️

expect(Reporter.properties).to.deep.equal([{ import: 'scipy' }]);
});

// That's probably enough different variants of code to verify nothing is wonky.

@IanMatthewHuff IanMatthewHuff Mar 13, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

// That's probably enough different variants of code to verify nothing is wonky. [](start = 3, length = 81)

Couple to consider if you want. You can have an import inside a function so it would be indented with leading whitespace. Also it violates PEP8, but via the definition you can multi import with , so like import numpy, pandas works. #Resolved

Comment thread src/client/extension.ts Outdated

// Activate import tracking
const importTracker = serviceManager.get<IImportTracker>(IImportTracker);
importTracker.activate();

@IanMatthewHuff IanMatthewHuff Mar 13, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks like it has the possibility of blowing up our extension activation time. It's line by line regex on every open document. #Resolved

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yeah I should at least check for python files and maybe limit the number of lines to say 1000.


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

@IanMatthewHuff IanMatthewHuff left a comment

Copy link
Copy Markdown
Member

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 0c581b2 into master Mar 13, 2019
@rchiodo
rchiodo deleted the rchiodo/import_telemetry branch March 15, 2019 16:34
@lock lock Bot locked as resolved and limited conversation to collaborators Jul 30, 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