Fix #138 Add medium level of IntelliSense / limit to the analysis depth (Related to #88). #146
Conversation
Limit depth analysys to relative paths which contains only 2 node_modules. This should be sufficient enough to capture a) Capture API of the root package b) Capture API of the dependencies. This is obiviously the must to analyze (1 node_modules) c) Capture API of subdependencies (2 node_modules). This is also the must, since a lot of packages are wrapper/glue for another library. For example using gulp-typescript require that gulp-typescript should be parsed (level b) and typescript should be parsed (level c) since typescritp provide some config options for gulp-typescript.
|
Hi @kant2002, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! TTYL, MSBOT; |
|
I add the configuration option for the Middle level of Intellisense (#138). Right now analysis level not configurable, but if you think this would be valuable, I could put additional config option for that. |
|
I'll take a look at this PR when I've had more sleep 😃. But lets make this configurable in the per-project setting as well - it shouldn't override the users general setting, but rather serve as an additional filter. This would enable users who are sharing a project to figure out good project-specific settings rather than relying on every single person on the team to use the same defaults. Because ultimately this is a project-specific perf issue. For the sake of UI simplicity, Let's hold off on making it configurable from general project settings unless/until we start getting requests for it. |
| <value>154, 21</value> | ||
| </data> | ||
| <data name="_mediumIntelliSenseRadioButton.TabIndex" type="System.Int32, mscorlib"> | ||
| <value>5</value> |
There was a problem hiding this comment.
Check the tab index here. WinForms is annoying in that it won't automatically update all the controls on the page.
|
Left some comments, minor tweaks for the most part. @DinoV, it would be great to get your eyes on this as well. |
| while (index != -1) { | ||
| nestedModulesCount++; | ||
| if (nestedModulesCount > limits.NestedModulesLimit) { | ||
| continue; |
There was a problem hiding this comment.
the check is nested now, so continue is going to continue the while loop, not the outer for loop, resulting an in infinite loop after the limit is exceeded.
One solution is the refactor the while loop and relevant variables out into its own private method, say "CheckExceedsNestedModulesLimit", and return true instead of continuing, false otherwise. Then...
if (!CheckExceedsNestedModulesLimit(path, limits)) {
files.Add(new AnalysisFile(file, File.ReadAllText(file)));
}
|
Hey there, just checking in to see if you have any questions. It would be great to get this change into the next release. 😃 |
|
Hi, was side-tracked on the main work. Will take a look at the comments from @DinoV later today, or tomorrow |
|
Yeah, no worries - it's totally okay to have a life outside NTVS 😉. This feature is super impactful, though, so unlike cookie dough... the more bake time, the better. |
oh yes, we have a good amount of developers waiting... (the homebrew patch is fine but it would be so much nicer to get rid of it) |
… but in the actual product code.
|
Thanks all for waiting, I send additional changes which should address @DinoV comment that I don't place fix in the correct place. |
| return false; | ||
| } | ||
|
|
||
| startIndex = index + 1; |
There was a problem hiding this comment.
this can be startIndex = index + "node_modules".Length since you already know you've seen node_modules at index
1. Use limit settings instead of constants. 2. Removed constants, moved explanation to the property initialization 3. Performe dependency depth check as last check, since it is slowest one.
| // | ||
| // All these examples are highly speculative and I specifically try to create such deep level. | ||
| // If you develop on windows with such deep level you already close to your limit, your maximum is probably 10. | ||
| // </remarks> |
There was a problem hiding this comment.
nit: there's an end tag, but no opening tag here. I don't believe would actually do anything here, so the tag should just be removed
|
Awesome, looks great! super nitpicky comment, but overall 👍 |
mousetraps
changed the title
Fix #138 Add medium level of IntelliSense / limit to the analysis depth (Related to #88). Limit depth analysys to relative paths which contains only 2 node_modules. This should be sufficient enough to capture a) Capture API of the root package b) Capture API of the dependencies. This is obiviously the must to analyze (1 node_modules) c) Capture API of subdependencies (2 node_modules). This is also the must, since a lot of packages are wrapper/glue for another library. For example using gulp-typescript require that gulp-typescript should be parsed (level b) and typescript should be parsed (level c) since typescritp provide some config options for gulp-typescript. Related to #88 #138 Added Medium level of Intellisense. This level now same as Full level, but limit analysis depth to the 2 nested modules depth. Full mode is now has limit to 4 modules depth which I almost sure practical enough, but could be increased if needed. Low level is analyse 1 level depth.
Limit depth analysys to relative paths which contains only 2 node_modules. This should be sufficient enough to capture
a) Capture API of the root package
b) Capture API of the dependencies. This is obiviously the must to analyze (1 node_modules)
c) Capture API of subdependencies (2 node_modules). This is also the must, since a lot of packages are wrapper/glue for another library. For example using gulp-typescript require that gulp-typescript should be parsed (level b) and typescript should be parsed (level c) since typescritp provide some config options for gulp-typescript.
Related to #88
#138 Added Medium level of Intellisense. This level now same as Full level, but limit analysis depth to the 2 nested modules depth. Full mode is now has limit to 4 modules depth which I almost sure practical enough, but could be increased if needed. Low level is analyse 1 level depth.