Skip to content

Conversation

@NickGuyver
Copy link
Contributor

No description provided.

@will-v-pi
Copy link
Collaborator

This change would break all backwards compatibility as that string is searched for to detect a pico-vscode project, and the header is also specified in pico_platform.py so would need to be changed there too. Could you modify this PR to address those concerns?

@NickGuyver
Copy link
Contributor Author

This change would break all backwards compatibility as that string is searched for to detect a pico-vscode project, and the header is also specified in pico_platform.py so would need to be changed there too. Could you modify this PR to address those concerns?

PR modified as requested.

Copy link
Collaborator

@will-v-pi will-v-pi left a comment

Choose a reason for hiding this comment

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

Thanks - couple of suggestions, but other than that looks good

Copy link
Collaborator

@will-v-pi will-v-pi left a comment

Choose a reason for hiding this comment

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

Actually one more change is needed in extension.mts, else it won't detect older projects as pico projects - this diff would work

diff --git a/src/extension.mts b/src/extension.mts
index 15b2b76..83f276e 100644
--- a/src/extension.mts
+++ b/src/extension.mts
@@ -16,6 +16,7 @@ import NewProjectCommand from "./commands/newProject.mjs";
 import Logger, { LoggerSource } from "./logger.mjs";
 import {
   CMAKE_DO_NOT_EDIT_HEADER_PREFIX,
+  CMAKE_DO_NOT_EDIT_HEADER_PREFIX_OLD,
   cmakeGetSelectedBoard,
   cmakeGetSelectedToolchainAndSDKVersions,
   configureCmakeNinja,
@@ -209,9 +210,14 @@ export async function activate(context: ExtensionContext): Promise<void> {
   // check if it has .vscode folder and cmake donotedit header in CMakelists.txt
   if (
     !existsSync(join(workspaceFolder.uri.fsPath, ".vscode")) ||
-    !readFileSync(cmakeListsFilePath)
-      .toString("utf-8")
-      .includes(CMAKE_DO_NOT_EDIT_HEADER_PREFIX)
+    !(
+      readFileSync(cmakeListsFilePath)
+        .toString("utf-8")
+        .includes(CMAKE_DO_NOT_EDIT_HEADER_PREFIX) ||
+      readFileSync(cmakeListsFilePath)
+        .toString("utf-8")
+        .includes(CMAKE_DO_NOT_EDIT_HEADER_PREFIX_OLD)
+    )
   ) {
     Logger.warn(
       LoggerSource.extension,

With this fix I've tested and am happy to merge this PR

fix by will-v-pi to detect older projects as pico projects
forgot deletion of old
@NickGuyver
Copy link
Contributor Author

Actually one more change is needed in extension.mts, else it won't detect older projects as pico projects - this diff would work

diff --git a/src/extension.mts b/src/extension.mts
index 15b2b76..83f276e 100644
--- a/src/extension.mts
+++ b/src/extension.mts
@@ -16,6 +16,7 @@ import NewProjectCommand from "./commands/newProject.mjs";
 import Logger, { LoggerSource } from "./logger.mjs";
 import {
   CMAKE_DO_NOT_EDIT_HEADER_PREFIX,
+  CMAKE_DO_NOT_EDIT_HEADER_PREFIX_OLD,
   cmakeGetSelectedBoard,
   cmakeGetSelectedToolchainAndSDKVersions,
   configureCmakeNinja,
@@ -209,9 +210,14 @@ export async function activate(context: ExtensionContext): Promise<void> {
   // check if it has .vscode folder and cmake donotedit header in CMakelists.txt
   if (
     !existsSync(join(workspaceFolder.uri.fsPath, ".vscode")) ||
-    !readFileSync(cmakeListsFilePath)
-      .toString("utf-8")
-      .includes(CMAKE_DO_NOT_EDIT_HEADER_PREFIX)
+    !(
+      readFileSync(cmakeListsFilePath)
+        .toString("utf-8")
+        .includes(CMAKE_DO_NOT_EDIT_HEADER_PREFIX) ||
+      readFileSync(cmakeListsFilePath)
+        .toString("utf-8")
+        .includes(CMAKE_DO_NOT_EDIT_HEADER_PREFIX_OLD)
+    )
   ) {
     Logger.warn(
       LoggerSource.extension,

With this fix I've tested and am happy to merge this PR

Fix applied, thank you.

@will-v-pi will-v-pi merged commit 9ca0df0 into raspberrypi:main Oct 31, 2024
1 check passed
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.

2 participants