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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Added the feature to auto-detect workspace directories and set it as default analyzedProjectDirectory. #83

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ca17c93
Added the feature to auto-detect first workspace directory and set it…
9brada6 Apr 19, 2020
cc63d30
Make phan auto-detect all project derectories from workspace folders,…
9brada6 Apr 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 43 additions & 14 deletions src/extension.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,17 +163,12 @@ async function checkPhanSupportsLanguageServer(context: vscode.ExtensionContext,

// Returns true if phan.phanScriptPath supports the language server protocol.
async function checkValidAnalyzedProjectDirectory(context: vscode.ExtensionContext, analyzedProjectDirectory: string): Promise<boolean> {
const exists: boolean = isDirectory(analyzedProjectDirectory);

if (!exists) {
if (!isDirectory(analyzedProjectDirectory)) {
await showOpenSettingsPrompt('The setting phan.analyzedProjectDirectory refers to a directory that does not exist. directory: ' + analyzedProjectDirectory);
return false;
}

const phanConfigPath = path.join(analyzedProjectDirectory, '.phan', 'config.php');
const phanConfigExists: boolean = isFile(phanConfigPath);

if (!phanConfigExists) {
if (!pathContainsPhanFolderAndConfig(analyzedProjectDirectory)) {
await showOpenSettingsPrompt('The setting phan.analyzedProjectDirectory refers to a directory that does not contain .phan/config.php. Check that it is the absolute path of the root of a project set up for phan. directory: ' + analyzedProjectDirectory);
return false;
}
Expand Down Expand Up @@ -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);
Copy link
Owner

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

Copy link
Owner

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

Copy link
Author

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.

Copy link
Author

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.

const enableDebugLog = conf.get<boolean>('enableDebugLog');
const analyzeOnlyOnSave = conf.get<boolean>('analyzeOnlyOnSave');
const allowPolyfillParser = conf.get<boolean>('allowPolyfillParser') || false;
Expand Down Expand Up @@ -255,13 +250,17 @@ export async function activate(context: vscode.ExtensionContext): Promise<void>
const phanScriptPathValidated = phanScriptPath; // work around typescript complaint.

// Check if the analyzedProjectDirectories setting was provided.
if (!analyzedProjectDirectories) {
if (originalAnalyzedProjectDirectories instanceof Array) {
await showOpenSettingsPrompt('The setting phan.analyzedProjectDirectories must be a path or a non-empty array of paths (e.g. /path/to/some/project_folder). `.phan` must be a folder within that directory.');
} else {
await showOpenSettingsPrompt('The setting phan.analyzedProjectDirectories must be provided (e.g. /path/to/some/project_folder). `.phan` must be a folder within that directory.');
if (!analyzedProjectDirectories.length) {
analyzedProjectDirectories = findValidProjectDirectories();

if (!analyzedProjectDirectories.length) {
// Do not send an error to the interface, this is frustrating.
Copy link
Owner

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

Copy link
Author

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.

const cantFindWorkspaceDirectoryMessage =
'No workspace directory contain a folder named ".phan" with a config.php file. ' +
'You can add custom directories via phan.analyzedProjectDirectories setting.';
console.warn(cantFindWorkspaceDirectoryMessage);
return;
}
return;
}

for (const dir of analyzedProjectDirectories) {
Expand Down Expand Up @@ -424,3 +423,33 @@ export async function activate(context: vscode.ExtensionContext): Promise<void>
context.subscriptions.push(createClient(dirToAnalyze).start());
}
}

// 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.
Copy link
Owner

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.

Copy link
Author

@9brada6 9brada6 Apr 19, 2020 •
edited
Loading

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.

function findValidProjectDirectories(): string[] {
// Get the fsPath(file system path) of all workspace folders.
const VSCodeFolders = vscode.workspace.workspaceFolders;
Copy link
Owner

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';)

Copy link
Author

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

if (!VSCodeFolders) {
return [];
}
let workingFolders = VSCodeFolders.map(function (obj) {
if (('uri' in obj) && ('fsPath' in obj.uri)) {
return obj.uri.fsPath;
}
return '';
Copy link
Owner

@TysonAndre TysonAndre Apr 19, 2020 •
edited
Loading

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)

Copy link
Author

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 '';

Copy link
Owner

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()

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed to just "return obj.uri.fsPath;", since "uri" will always be in "WorkspaceFolder", check here, and "fsPath" will always be in uri: here

});

workingFolders = workingFolders.filter(function (folderPath) {
return pathContainsPhanFolderAndConfig(folderPath);
});

return workingFolders;
}

// Whether or not a path contains the ".phan" folder alongside with a config.php file.
Copy link
Owner

@TysonAndre TysonAndre Apr 19, 2020 •
edited
Loading

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

// Returns bool
function pathContainsPhanFolderAndConfig(folderPath: string): boolean {
let phanConfigPath = path.join(folderPath, '.phan', 'config.php');
return isFile(phanConfigPath);
}