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

Add depth limit configuration option #2

Closed
martinpengellyphillips opened this issue Mar 31, 2016 · 13 comments
Closed

Add depth limit configuration option #2

martinpengellyphillips opened this issue Mar 31, 2016 · 13 comments

Comments

@martinpengellyphillips
Copy link
Contributor

martinpengellyphillips commented Mar 31, 2016

Consider adding a way to limit the search depth (configuration option?) in order to improve performance for large folders.

@felipecaputo felipecaputo added this to the 0.1.4 milestone Mar 31, 2016
@felipecaputo felipecaputo self-assigned this Mar 31, 2016
@felipecaputo felipecaputo modified the milestones: 0.1.4, 0.1.5 Mar 31, 2016
@felipecaputo
Copy link
Owner

Version 0.1.5 released with maxDepthRecursion and ignoredFolders config. I hope it will help you.

@martinpengellyphillips
Copy link
Contributor Author

Thanks for this, but I didn't see any performance boost. Looking at master I see the filterDir code commented out - is that intentional / in the release?

https://github.com/felipecaputo/git-project-manager/blob/master/src/gitProjectLocator.js#L53

Also, I think the max depth might work better if the search didn't have to recurse into the .git directory to find it. Instead it would check each directory for a subdirectory of .git. This would allow a depth of 1 to very quickly find projects that are all stored under a common directory without having to also specify the configuration to exclude folders like node_modules.

@felipecaputo
Copy link
Owner

@martinpengellyphillips Sorry for that, I forgot that vcse publishs the local code instead of commited code. I Will fix that ASAP.

And I will make the .git fix in the same release

@felipecaputo
Copy link
Owner

Solved in 0.1.6.

Sorry again @martinpengellyphillips, but now i think it is working fine

@martinpengellyphillips
Copy link
Contributor Author

Thanks for the quick update!

Since updating to 0.1.6 the extension breaks for me with the following error:

threadService.ts:199 [Plugin Host] WARNING: Promise with no error callback:67
threadService.ts:199 [Plugin Host] Object {exception: null, error: Object, promise: Object, id: 67}
threadService.ts:199 [Plugin Host] WARNING: Promise with no error callback:68
threadService.ts:199 [Plugin Host] Object {exception: null, error: Object, promise: Object, id: 68}
shell.ts:480 An unknown error occurred. Please consult the log for more details.e.onUnexpectedError @ shell.ts:480(anonymous function) @ shell.ts:342e.onUnexpectedError @ errors.ts:72u @ errors.ts:87e.onUnexpectedPluginHostError @ extHost.api.impl.ts:469e.handle @ abstractThreadService.ts:100c @ ipcRemoteCom.ts:172f @ ipcRemoteCom.ts:120doNTCallback0 @ node.js:420_tickCallback @ node.js:349
shell.ts:480 e.text.substr is not a function: TypeError: e.text.substr is not a function
    at e.<anonymous> (file:///C:/Program Files (x86)/Microsoft VS Code/resources/app/out/vs/workbench/workbench.main.js:41:17181)
    at e.doElement (file:///C:/Program Files (x86)/Microsoft VS Code/resources/app/out/vs/workbench/workbench.main.js:39:17966)
    at e.li (file:///C:/Program Files (x86)/Microsoft VS Code/resources/app/out/vs/workbench/workbench.main.js:39:17235)
    at e.renderMessage (file:///C:/Program Files (x86)/Microsoft VS Code/resources/app/out/vs/workbench/workbench.main.js:41:16405)
    at file:///C:/Program Files (x86)/Microsoft VS Code/resources/app/out/vs/workbench/workbench.main.js:41:16181
    at Array.forEach (native)
    at e.<anonymous> (file:///C:/Program Files (x86)/Microsoft VS Code/resources/app/out/vs/workbench/workbench.main.js:41:16157)
    at e.doElement (file:///C:/Program Files (x86)/Microsoft VS Code/resources/app/out/vs/workbench/workbench.main.js:39:17966)
    at e.ul (file:///C:/Program Files (x86)/Microsoft VS Code/resources/app/out/vs/workbench/workbench.main.js:39:17111)
    at e.renderMessages (file:///C:/Program Files (x86)/Microsoft VS Code/resources/app/out/vs/workbench/workbench.main.js:41:16022)

I'll pull master locally and try to debug this further unless it is something you have seen already?

@felipecaputo
Copy link
Owner

In debug the error is not happening, I'm trying to figure it out. Any help is welcome

@felipecaputo
Copy link
Owner

VSCE does not manage well dependencies of dependencies :(

I've figured it out in the worst way, but fixed now in 0.1.9

Sorry for the inconvenience @martinpengellyphillips

@martinpengellyphillips
Copy link
Contributor Author

Works! Thanks.

One thing about max depth - I think it would be best if the max depth check is inclusive rather than the current exclusive (now that the .git subfolder check is performed):

 return (maxDepth > 0) && ((currentDepth - initialDepth) > maxDepth);

to

 return (maxDepth > 0) && ((currentDepth - initialDepth) >= maxDepth);

This would then ensure that the common case of projects directly under projects base path is handled efficiently.

Thanks again!

@martinpengellyphillips
Copy link
Contributor Author

Nevermind my last comment - max depth does work as expected!

I was just expecting more of a performance boost, but I see it is actually git remote show origin -n that is taking the time.

@felipecaputo
Copy link
Owner

What do you think about get the origin being optional?

There isn't a real necessity for that, it was just an idea of handling cases with projects with the same name but, the label is the path itself so, we can let the repo origin configurable.

@martinpengellyphillips
Copy link
Contributor Author

Yeah - I think that would make sense. For me, the main thing about this extension was to reuse the fact I have .git for all my projects already, so it is a great way to discover projects quickly. I don't really care that much about the origin when looking for a project - I would drop it altogether ;-)

On that note, it might be good to also consider putting the project name first and then the path after (perhaps like you had origin).

project-a /full/path/to/groupa/projects
project-b /full/path/to/groupa/projects
project-c /full/path/to/groupb/projects

This would make the most important information (project name) more visible.

I guess doing that could also help with filtering if name is considered first and then you can narrow down based on path name. So in the following (contrived) example, you would only have to type a rather than artis to select the first project:

/Users/martin/projects/artisan
/Users/martin/projects/bett
/Users/martin/projects/bungle

@felipecaputo
Copy link
Owner

felipecaputo commented Apr 22, 2016

@martinpengellyphillips I've added the configuration to disable the check remote origin, sorry for taking too much time but last sprint was hard to finish

I've forgot the option to show project in the start of the quick pick, I will open a new issue for that and address that in 0.1.11

@martinpengellyphillips
Copy link
Contributor Author

Thanks @felipecaputo ! This works perfectly - with storeRepositoriesBetweenSessions set to false, maxDepthRecursion set to 1 and checkRemoteOrigin set to false, my large list of projects over three roots loads instantly whereas before it would take up to a minute. Thanks again for being so responsive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants