-
Notifications
You must be signed in to change notification settings - Fork 4
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
Added the feature to auto-detect workspace directories and set it as default analyzedProjectDirectory. #83
base: master
Are you sure you want to change the base?
Conversation
… as default analyzedProjectDirectory. analyzedProjectDirectory is no longer needed to be explicit set.
… and do not show alerting errors if no directory was found.
analyzedProjectDirectories = findValidProjectDirectories(); | ||
|
||
if (!analyzedProjectDirectories.length) { | ||
// Do not send an error to the interface, this is frustrating. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's also frustrating to install an extension for a tool you're unfamiliar with and have it not work (e.g. some users may try to start using the extension before setting up a Phan config for the project).
I'd rather you added a config setting such as phan.silenceNoDirectoryWarning: bool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, sounds good, but also think about this: when you install an extension, you go to the vscode extension page. There, one of the first thing a user must see is this:
"For this extension to work, a workspace folder must contain the ".phan" folder with a"config.php" file inside. To add custom folders and override auto-detection use the setting "analizedProjectDirectories" ".
Having a bunch of configs for every control structure I don't think it is a good idea. An extension should be pleasant and simple to use. Additionally we can alert an error when the setting to debug is on, this is when an user could not figure out how to use, and enabling this will throw the error.
Also I think analizedProjectDirectories should be also make to detect relative paths, but that's another story.
if (('uri' in obj) && ('fsPath' in obj.uri)) { | ||
return obj.uri.fsPath; | ||
} | ||
return ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be clearer as a for loop - For example, this should filter out the objects without fsPath instead of using the empty string (Is that treated like the current working directory? I can't tell)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah,I think that the if is unnecessary, but I did that to be sure to not throw an exception accessing a undefined property. Changed to:
const filePathExistInObject = ( ('uri' in obj) && ('fsPath' in obj.uri) );
if ( filePathExistInObject ) {
return obj.uri.fsPath;
}
return '';
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean this would be clearer as a for loop (written in an imperative style) instead of a call to map() and filter()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return workingFolders; | ||
} | ||
|
||
// Whether or not a path contains the ".phan" folder alongside with a config.php file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be just // Returns true if a path contains ".phan/config.php"
"alongside with" could be read as containing both $PROJECT/.phan
and $PROJECT/config.php
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
|
||
// Search all the workspace folders and return the first that contains .phan/config.php | ||
// This will return the folder name in an array as required, and just only one folder, | ||
// the first met. We can easy modify the function to add all valid workspace folders to the array. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is out of date - this is already returning all valid workspace folders that were open when this extension started (or is unpaused?). I also see onDidChangeWorkspaceFolders exists in https://code.visualstudio.com/api/references/vscode-api#workspace , but that's out of scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is running when extension is activated, so it runs with activate() function. Rewrite the comment to be:
// Search all the workspace folders and return the ones that contains .phan/config.php file.
The function makes exactly what the title says. Maybe in the future add a new function like "handleWorkspaceFoldersChanged()" and bound to the event 'onDidChangeWorkspaceFolders', that will call findValidProjectDirectories() and updated/restart the extension.
@@ -203,7 +198,7 @@ export async function activate(context: vscode.ExtensionContext): Promise<void> | |||
const phanScriptPath = conf.get<string>('phanScriptPath') || defaultPhanScriptPath; | |||
// Support analyzing more than one project the simplest way (always start every server). | |||
const originalAnalyzedProjectDirectories = conf.get<string|string[]>('analyzedProjectDirectory'); | |||
const analyzedProjectDirectories = normalizeDirsToAnalyze(originalAnalyzedProjectDirectories); | |||
let analyzedProjectDirectories = normalizeDirsToAnalyze(originalAnalyzedProjectDirectories); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option I thought of would be to have the magic value *
for phan.analyzedProjectDirectory mean that this extension should attempt to open any workspace with a valid phan config (in addition to hiding any showOpenSettingsPrompt calls).
Or just a boolean phan.enableForAllProjects: true
, which is equivalent.
If that was enabled, then nothing would be popped up. If that was disabled, then phan would show the existing popup, but mention the new alternatives for making the popup not show up
That way, it'd be possible both to force Phan to start a server for a few directories, and to analyze the currently open directores (e.g. in case a workspace had multiple phan configs, or a user occasionally opened different projects)
Still, I'd really rather not enable this by default
- Projects may have their own plugins in
.phan/config.php
which aren't compatible with the version of phan bundled with vs code (or configured by the user in vs code settings for other projects) - Projects may use Phan configuration settings that are extremely, extremely slow or memory intensive, or contain files that take a long time to parse/run plugins on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If analyzedProjectDirectory was merged with findValidProjectDirectories(), the list of folders would need to be deduplicated, preferring the configured folder name.
https://nodejs.org/api/fs.html#fs_fs_realpathsync_path_options is one way, in case there are symlinks or different ways to represent the same folder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand exactly what you mean here, I will try tomorrow, maybe there is a better solution for this to be worked out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If analyzedProjectDirectory was merged with findValidProjectDirectories(), the list of folders would need to be deduplicated, preferring the configured folder name.
This I don't also understand, analyzedProjectDirectory is a setting and findValidProjectDirectories() is a function, if anything is passed into analyzedProjectDirectory, even a string that is not a path, then findValidProjectDirectories() will not run at all.
// the first met. We can easy modify the function to add all valid workspace folders to the array. | ||
function findValidProjectDirectories(): string[] { | ||
// Get the fsPath(file system path) of all workspace folders. | ||
const VSCodeFolders = vscode.workspace.workspaceFolders; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: vscodeFolders would be more consistent with other names - uppercase looks like a class (e.g. import { DocumentFilter, RelativePattern } from 'vscode';
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I will change it to "vscodeFolders" if "vscode" is a standard
Hi, what do you think about this implementation: phan.analyzedProjectDirectories = bool
string|string[]
|
A setting of just
Not sure which setting that comment is referring to (php version used to run phan?). It seems like an extremely uncommon use case to install php within the project being run (and not cross platform), so I'd rather not complicate phan by adding that If a user has multiple directories, ./external/vendor/phan/phan/phan might not work as well. Maybe a hash map from directories to relative or absolute path overrides to use for Phan.
Good to know, will check out those extensions and consider - would need to check versions for some minimum compatible version if newer CLI options such as |
I don't think you understand what I was saying. The php thing was a bad example and you didn't understand, sorry. TLDR: In short, Instead of writing an absolute path(C:\my_projects...) to the project, just write the workspace name between the %%, and if we do this then the project can easily be moved between different computers. Let's make some uses case:
For 1 we do: "phan.analyzedProjectDirectories": true. (auto-analizing) And the path can also be relative to workspace name: "%NameOfWorkspace1%/../../a_folder/" |
Also, a workspace name is the one set in the .code-workspace setting: For vscode folder settings, we have relative paths: "phan.analyzedProjectDirectories": "./my_folder/". |
You will need to downgrade vscode-php-phan to 3.0.0 for your workflow to continue working (and can use phan.phanScriptPath to point to newer phan installations) - I had meant to forbid it entirely but could not even figure out if vs code provided a mechanism to disable user and workspace settings when I was working on this years ago. I know this is inconvenient - I'd been discussing how this could be done in a user-friendly and safe way with the reporter of CVE-2021-31416 For current vscode versions:I looked into how
For future vscode versionsmicrosoft/vscode#106488 is something along the lines of what I wanted (with respect to phan configs and phan plugins being executable code, etc)
|
https://code.visualstudio.com/updates/v1_57 VS Code 1.57 added that in May, though 106488 is still open and the functionality may change. |
Added the feature to auto-detect workspace directories and set it as default analyzedProjectDirectory. analyzedProjectDirectory is no longer needed to be explicit set.
Other Improvement:
If no folder project for Phan was found, then only show a console warning, this will make Phan extension to be able to remain all time enabled, instead of being always disabled/enabled per workspace to not show unwanted interface alerts.
If "analyzedProjectDirectory" setting is set and the folder is not valid, it will trigger an interface alert like before.
Testing:
Tested manually on Windows 10, with no workspace containing .phan, one workspace containing .phan, and 2 containing, and it works. To fix #79 and #29. Maybe need to test on Linux/Mac, but it should work.
Modifications:
Firstly, I added the function findValidProjectDirectories() that will return first valid workspace directory, this can be easily redesigned to return all valid directories.
To valid a directory, I added a function pathContainsPhanFolderAndConfig(): bool, based on a similar approach in the function checkValidAnalyzedProjectDirectory and reformat for DRY.
I modified the output log inside the if (!analyzedProjectDirectories) {}, to add a more consistent console message. Note that analyzedProjectDirectories was always an array(which is an object in JS), so that the control structure was always false and not run. Using a console phan will not need to be disabled for non-PHP projects because of unwanted interface alerts(will trigger an alert only when analyzedProjectDirectory was explicitly set).