-
Notifications
You must be signed in to change notification settings - Fork 335
[VSC-1578] Modify Event Activation for file types #1568
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
Conversation
|
Download the artifacts for this pull request: |
brianignacio5
left a comment
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.
We need to consider multiple projects scenario.
brianignacio5
left a comment
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.
LGTM
|
Pull request has been marked as |
Removed previous activation events for file types.
We only check for CMakeLists.txt at root project folder if it includes "include($ENV{IDF_PATH}/tools/cmake/project.cmake)"
- No matter the order of the projects, if an idf extension is present, extension will activate itself. - If no idf project is detected, but CMakeLists.txt is detected at root level user is asked if he wants to activate extension
fix: lint issue
If there were folders without CMakeLists.txt file in root, but they had a CMakeLists.txt file in there subfolders, the extension would have activated
Refactored the activation check to search for any CMakeLists.txt in the workspace and verify if it is an ESP-IDF project, rather than only checking root folders. Updated localized messages in Spanish, Portuguese, Russian, and Chinese to reflect the new logic and provide clearer prompts to users.
d6ab91c to
254747e
Compare
Fabricio-ESP
left a comment
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.
Confirmed extension tracks CMakeFiles.txt.
Note file is case sensitive.
brianignacio5
left a comment
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.
The PR validates CMakeLists.txt only. Using "**/{build,node_modules}/**" seems unnecessary.
src/extension.ts
Outdated
| if (PreCheck.isWorkspaceFolderOpen()) { | ||
| const cmakeListsFiles = await vscode.workspace.findFiles( | ||
| "**/CMakeLists.txt", | ||
| "**/{build,node_modules}/**" |
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.
Why do we need node_modules? Isn't it from JavaScript Typescript projects ?
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.
The build folder is also used in CMake project or many other kind of projects. The next validation is only for CMakeLists.txt so I'm just wondering why other files are needed here.
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.
The vscode.workspace.findFiles expects as a second argument an exclusion pattern. I just thought it would be good to ignore searching in file such as "node_modules" and "build", if they are present in the opened workspace.
This was the exact thing I wanted to discuss about this new implementation: I can remove the "node_modules" if you think it's unnecessary or add these exclusion patterns for other languages as well, to increase performance of vscode.workspace.findFiles
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.
Oh I see.
Remember that the extension is activated either by running command or "workspaceContains:**/CMakeLists.txt"
Later you can use loop of all workspaceFolders with a relative pattern per workspace folder validation for root only:
const relativePattern = new RelativePattern(workspaceFolderUri, "CMakeLists.txt");
const files = await workspace.findFiles(relativePattern);The content you are validating is for ESP-IDF root CMakeLists.txt only, so no need to check additional places.
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 have changed the implementation once again based on our discussion only needing the CMakeLists.txt from root level. There is no need to use workspace.findFiles() anymore. I've left more details in the commit description
- Replace recursive CMakeLists.txt search with root-only validation - Use direct file system checks instead of workspace.findFiles for better performance - Support multiple workspace folders with ESP-IDF projects at root level - Maintain user prompt for non-ESP-IDF CMakeLists.txt files
brianignacio5
left a comment
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.
LGTM
Removed previous activation events for file types:
We only check for CMakeLists.txt at root project folder if it includes
"include($ENV{IDF_PATH}/tools/cmake/project.cmake)"Description
Please include a summary of the change and which issue is fixed.
Fixes #1414
Type of change
Please delete options that are not relevant.
Steps to test this pull request
a. If it contains a
CMakeLists.txtat the root level which doesn't contain the textinclude($ENV{IDF_PATH}/tools/cmake/project.cmake)extension should ask, using a notification, if you want to activate esp-idf extension anyway.b. If it doesn't contain a
CMakeLists.txtat all, it should not activate automatically.How has this been tested?
As described above
Test Configuration:
Checklist