-
Notifications
You must be signed in to change notification settings - Fork 8
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 multiple config files based on a file pattern (different to order of precendence) #107
Comments
I do like solution 2 a lot more since it requires a lot less user effort and is less likely to go wrong. But I do have to say that I'm not super opposed to the idea anymore. A few challenges still that I'll have to think about and that this kind of hinges on:
These are kind of the two biggest implementation issues. If it turns out that one of them is just really hard to find a solution for (or it is possible but really complex) then it's probably not worth it to implement this. If they turn out to be doable then it should be rather easy to implement. So TLDR: I am open to the idea but will have to think about it a bit. Might take a while before this is implemented though since I am a bit busy in the coming weeks and since bug-fixes obviously take priority. |
I'd prefer the mutually exclusive option to not impact current user setups (I'm pretty sure I have seen other extensions do this, so that your I would say "phpstan.configFiles": [
"${workspaceFolder}/src.phpstan.neon, ${workspaceFolder}/src.phpstan.neon.dist, ${workspaceFolder}/src.phpstan.dist.neon",
"${workspaceFolder}/test.phpstan.neon, ${workspaceFolder}/test.phpstan.neon.dist, ${workspaceFolder}/test.phpstan.dist.neon"
], IE. having the below configured in "phpstan.configFile": "${workspaceFolder}/src.phpstan.neon, ${workspaceFolder}/test.phpstan.neon",
"phpstan.configFiles": [
"${workspaceFolder}/src.phpstan.neon",
"${workspaceFolder}/test.phpstan.neon"
],
It could be worth just having a small PHP script that will take a The compute shouldn't be that taxing at all for this, so shouldn't impact run time and could probably be done as a pre-task to every run of the extension. As much as it pains me, I do understand the low priority. Thank you for all the effort, know that it is very much appreciated. |
Hmm yeah I agree that using an array of comma-separated values is probably the best. I'm also considering just switching to
Maybe good to get the scenarios as to which one is evaluated when clear:
I'm a bit wary of this for a couple of reasons:
I completely agree that this would be ideal and would be a great match to what PHPStan does, but I'm thinking this is likely to cause a lot of edge cases with system setups to pop up. Maybe simpler to just build a JS implementation of the neon parser (possibly porting it with ✨AI✨) and keep it all in-extension. |
Well I've just found a neon parser in JS so that's no longer a problem. That's pretty nice |
I've implemented it and I think it works (tested it with the new |
Sorry for the late response! For me whilst this does work, it doesn't really suit my purpose since I have certain folders as set up as different workspaces in vscode. I see that this extension doesn't support multi-root workspaces (and only supports the first workspace shown), but was wondering if we could have an override to enable per-workspace scans? My phpstan config files point to these different roots, so even if we ran all of these phpstan processes on different workspaces at the same time: I wouldn't have any crosstalk. But it is not possible to do so atm. I can see why you didn't support multi-root in the first place, with cache invalidation probably being a big drive, but if you configure your stan configs correctly to point to the right directories, then it should just work. |
Hmm that does complicate things a bit, largely because configuration right now is all relative to the current workspace. But it may be doable to fit this into the current model. As multiple configs are now supported in this branch, it uses the currently scanned file to determine the neon file. It's not a massive stretch to also use that file to determine its workspace folder to base the config off of. Some questions though to understand:
|
I do have some projects with multiple neon files in them for scanning different directories of the same project. Wheras I have a separate application that is still a part of the same overall project, so it is a separate root workspace in vscode. https://code.visualstudio.com/docs/editor/multi-root-workspaces. I could place all of my .neon config files from all of my projects in the new "phpstan.configFiles" setting, however only changes in the first workspace root will trigger this extension to start scanning. Wheras I would be looking for an override to allow it to trigger on changes to any workspace root. in my workspace: ...
"folders": [
{
"name": "primary",
"path": "../primary"
},
{
"name": "secondary",
"path": "../secondary"
},
],
... Any changes in the "../primary" directory will trigger phpstan scans, this will also scan the "../secondary" directory as expected (due to the new and shiny multi-config file support) - However nothing you ever do in the "../secondary" directory could trigger the extension to start a scan, even if your setup could handle it.
Most extension handle this with path mappings. For instance: "phpunit.paths": {
"${workspaceFolder:primary}": "/var/www/primary",
"${workspaceFolder:secondary}": "/var/www/secondary",
} It could be something like: "phpstan.paths": {
"${workspaceFolder:primary}": [
"config/primary.1.neon",
"config/primary.2.neon",
"config/primary.3.neon"
],
"${workspaceFolder:secondary}": [
"config/secondary.src.neon",
"config/secondary.tests.neon"
]
} Although just allowing the scan to be triggered from any workspace root sounds like a simpler solution that would also work for me :) |
The new version actually doesn't seem to work as expected in terms of this feature, the scan never completes when I use more than one config, when each works individually on their own. See my attached video 2024-10-07.11-56-43.mp4 |
I've just added multi-workspace support, I think it works (I've tested it on a demo project and it worked). Could you give it a try? Regarding the testing not succeeding when using multiple configs, could you send along the logs of the extension ( Regarding my questioning of how to configure this, I did not know that VSCode allows for per-workspace settings that would then space multiple workspace roots. So indeed configuring it in the workspace settings is entirely fine. I've right now allowed for |
Ah yes, thanks for the report. Both of those should be fixed in this version: |
More of the same unfortunately, works fine with the configs individually, then when both are entered it is perpetually spinning in the task tray with no real indication of anything going on in the console (where usually I can see the % scan going up, or instantly complete). Nothing happens until I remove one of the configs then ctrl+s another php file, when it will catch up and run against the single config just fine. |
I've got a hunch as to what this might be. I've added some logging in this new version, could you run this and send me the logs again? It's important to check the logs for |
I just started looking into support for multiple config files and I stumbled upon this issue Currently I work on a big laravel monorepo where there are a lot of folders in To solve issue of PHPStan being slow for the whole project and using a lot of memory we decided to add
I use this extension and I manually change I was wondering would it be possible for this extension to automatically detect closest For example, if I'm editing
|
@SanderRonde I'm willing to test the |
Thanks! If yoy rename the file to a .vsix file you can drag it into the extensions panel or hit the "install from vsix" button in the dots menu |
I initially missed your earlier comment. I do like the idea of resolving based on current location. It's pretty close to what typescript, eslint and prettier do. I'll try to look into this idea some more since indeed configuring every neon file location for every package in a monorepo is quite terrible, and having it "just work" would be really nice. (there might be some hurdles/issues to overcome but I'll see :)) |
Auto resolving configs would be really nice. Thanks for taking that into consideration I've managed to install the extension. Thanks for the help! I've also checked out the As I mentioned before, my work monorepo is already setup with few I never published vscode extension but I do know basic stuff about debugging extension so maybe I can also help with that if you need any output Let me know how I can help with this |
I've implemented it in the
Apart from that all resolving is done automatically. The current steps are:
You can also keep tabs on whether resolving succeeded and which config file was found by hovering over the "language status" (as VSCode calls it) status bar item. You'll recognize it as the I went for finding all files in the workspace over walking up the tree since:
Would you mind testing it out on your monorepo? (@Sectimus I'm fairly certain this will work better in your case as well, basically achieving the same with way less configuration) |
Thanks for the quick update. I just tested the 3.2.14. I installed the extension and then I reloaded the window. In the status bar I see Here is the
After around 5min I see that
and
After 10 min "Language status" still shows EDIT: after 11min "Language status" is gone but there is no output for server or client |
When I change and save some file same check starts again |
In this comment you mention
What would be the order of config files to check for path? Will it
|
Ahh not sure about the check timing out, will debug that later. Do you maybe have a repo on which I can reproduce it or is it not public?
This is not in any particular order. It's the order in which vscode returns the |
Unfortunately repo is private. I'll check out the I think ideally same path shouldn't be in 2 neon files. If you implement it that way it would be easy to tweak neon files to follow that. I would prefer sub-folder up to root approach but that's just something that fits my case nicely |
Thanks a bunch! I think the easiest way to debug it is to find the logs and put a breakpoint there. For example it looks like it's stuck at the check-start log. So I'd put a breakpoint there and step through to see where it ends. Important to launch the
Not sure if this is necessary. I'm pretty sure VSCode already only includes files in the workspace (so not ignored files like vendor/ and node_modules/) so it should be relatively performant and correct. Regarding the order in which checking happens, I do think that checking in a "random" order (determined by VSCode) is not the best. Will add some order of resolving later. Indeed probably bottom-up fits nicely. |
Hey, sorry for the delay. I just got back to testing this Second place that you proposed in I've put the breakpoint in check-start log and it goes to this line const result = await runningCheck.promise; It spends most of the time on that line and after some time it goes to next line and I see
I then changed logic in files: (await workspace.findFiles(params.pattern, '**/vendor/**', 1)).map( with this change it only finds one single When I run that single phpstan config manually in terminal there is only 77 files to scan and it runs pretty fast - maybe in 5 seconds I also checked what
I found following github pages for those files https://github.com/thephpleague/oauth1-client and composer require 'jean85/pretty-package-versions:^1.5 || ^2.0'
composer require league/oauth1-client
composer require dragonmantank/cron-expression Let me know where should I look next |
Thanks for looking into it, indeed those files were not being ignored, should be fixed now. (the docs even mention that these files are not excluded so my bad) Regarding it stalling on that promise. That promise is the big promise that captures the entire check, could you put a breakpoint here? That is the actual contents of the check. Thanks so much! |
Thanks for your help so far! I had issue with debugging being really slow so I had to add "debug.inlineValues": "off", to my I did some debugging and I found 2 issues and once I fixed those phpstan managed to run successfully First, in one of the package folders I had Second, I debugged until const parameters = neonFile.get('parameters'); and after few loops it would never reach line 72. I managed to find that it was parsing rules:
- PHPStan\Rules\PHPUnit\AssertSameBooleanExpectedRule
- PHPStan\Rules\PHPUnit\AssertSameNullExpectedRule
- PHPStan\Rules\PHPUnit\AssertSameWithCountRule
- PHPStan\Rules\PHPUnit\MockMethodCallRule
- PHPStan\Rules\PHPUnit\ShouldCallParentMethodsRule I noticed that it doesn't have parameters: so I added empty I didn't see any errors for missing |
Awesome, thank you so much! Will fix this bug. It should indeed at least warn of a missing parameters field instead of silently failing. Also good to know about the debugHints thing since I tend to step through and debug quite often as well. |
Fixed the parameters issue, thanks! |
Thanks for fixing that and for working on this multi config feature! I really appreciate it! I checked the readme and saw that I can use |
Yes indeed, thank you! |
|
thanks but i think you mentioned the wrong issue, it should be this one ? #111 |
Hehe indeed, oops |
@SanderRonde Whilst this is great, I'm not so sure that this is able to solve my problem. Our PHPStan neon files make liberal use of Ie something like (
and we'll have a different config to isolate the large memory usage required for complex structures like (
With this new system, it will never work since it will always call the same neon file regardless since all neon files are always present and inside the exact same directory as one another. Ex:
I would be looking to execute |
Oops wrongly closed this issue. I think your configuration should work with this new approach too right? Given this configuration: "phpstan.configFile": "lightweight_scan.neon,heavy_scan.neon" It would find both the lightweight and heavy scan configs, then when resolving a change in |
This is the exact configuration from my comment 🤭
When I ran this, it only ever executed
I would think for it to achieve this, the extension would need to parse the |
It should be taking both |
Looks like it works when I'm testing it (added a test case here), what build were you testing? I could imagine me wrongly marking this as fixed gave the impression that this was live on the store-version of the extension. |
To prevent some unneeded back and forth, here is the latest build: |
Using that specific version, I can't get a single config file to run. [12/23/2024, 8:02:17 AM] [file-watcher] Checking: Document changed It doesn't tell me where this error occurred. I can run phpstan analysis just fine using the same config file. Any ideas? |
This happens even if the neon file itself is empty. Though I do get an error if the neon file is missing, so it's not complaining about that. |
Ah that sucks, here's another build that should log the location: Could you maybe post the contents of the |
This issue exists to diverge this discussion to it's own issue
The problem:
There is currently no way to allow this extension to use a different config file based on directory of the file that was saved.
Solution 1:
I propose a new setting where we can map our configs to directories.
this example would be mutually exclusive with
phpstan.configFile
Solution 2:
We could instead just read the parameters.paths & paramerers.excludePaths values in the PHPStan config files, since it is just using the
fnmatch()
function to interpret them, we could do the same process to receive the same result without needing to define a new setting.The text was updated successfully, but these errors were encountered: