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 support for globs in nim.project setting #90

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Aldlevine
Copy link

Instead of listing out all project files in vscode settings, this lets you specify glob patterns to match multiple files.

{
  "nim.project": ["src/**/*.nim"]
}

@kosz78
Copy link
Collaborator

kosz78 commented May 25, 2018

Using this approach it is very easy make mistake to include dont required project files, User should be very carefull with including project file because each project file creates separate nimssuggest process that consume a lot of memory, also for when the plugin perform linting it execute nim check for each project file.
I guess include each project file separately give more control than patterns

@Aldlevine
Copy link
Author

I see that now. I just tested this out on a project with ~16 files and it is very resource heavy. I agree that it's probably not best to have such a feature, as it would only be usable for small projects where it would have the least benefit.

That said, I did notice that if you specify a single source file then all imported modules appear to get checked as well, and it is very performant in comparison. What if the extension generates a temp source file which imports the matched modules and runs everything against that file? It's certainly not an elegant solution, but it seems to me like it might work. If that sounds like a reasonable option, I'd be happy to update this PR when I have time.

@saem
Copy link

saem commented Aug 19, 2020

For those discovering this much later, the reason why this hasn't been accepted or moved forward is because it runs counter to how Nim tooling works. Note, I don't understand all the details and am new to Nim and the extension's code (learning bits and pieces as I port it).

The way I've seen Nim work is that the file provided to Nim, nimsiggest, etc... Are an entry point akin to say webpack or JavaScript bundler entry point. From there imports describe the modules that need to be in scope for consideration, be it compiling or providing suggestions.

I believe @kosz78 is alluding to that above when they say each file starts a nimSuggest process. I believe this PR should be closed or changed to improving the docs for the setting and/or changing the name so it's clearer as to the intention of the existing setting, something like project entry point files. Would make a great easy first change for someone.

I realize because many modules will act as a library or a main module flip to tests or some sort of simple driver, that should be handled as a separate matter which I believe is covered by "run this file". The features that might be desirable in that space are likely pattern based defines/settings/etc that are passed to nimSuggest or what not so it does the right thing and can keep a smaller pool of processes in perhaps an LRU fashion so as to not burden the system and still provide the features people desire.

The above would also cover usecase where nimSuggest should respect defines such as is when, like myself, and working on a strictly is target project.

@RSDuck
Copy link
Contributor

RSDuck commented Aug 19, 2020

the intended way to use nimsuggest is to only have a single instances. To allow this nimsuggest may be started without a project file and tries to give results to files unrelated to the current project, though this was implemented before nimsuggest support was initially implemented in this extension. As a workaround the infamous spawning one nimsuggest instance per file was implemented, which was kept till today.

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

Successfully merging this pull request may close these issues.

4 participants