feat: usage analytics report for project types status - ON HOLD - DO NOT MERGE!!!#315
feat: usage analytics report for project types status - ON HOLD - DO NOT MERGE!!!#315NetaMolcho wants to merge 5 commits intomainfrom
Conversation
| if (isBAS()) { | ||
| AnalyticsWrapper.createTracker(context.extensionPath); | ||
| void reportProjectTypesToUsageAnalytics(basToolkitAPI); | ||
| } |
There was a problem hiding this comment.
| if (isBAS()) { | |
| AnalyticsWrapper.createTracker(context.extensionPath); | |
| void reportProjectTypesToUsageAnalytics(basToolkitAPI); | |
| } | |
| setTimeout(() => { | |
| if (isBAS()) { | |
| AnalyticsWrapper.createTracker(context.extensionPath); | |
| void reportProjectTypesToUsageAnalytics(basToolkitAPI); | |
| } | |
| }); |
There was a problem hiding this comment.
consider to move this code up into the first "if (isBAS())" code branch.
There was a problem hiding this comment.
I can't because I need to use basToolkitAPI which is created after the first isBAS()
| } | ||
|
|
||
| export function shouldRunCtlServer(): boolean { | ||
| export function isBAS(): boolean { |
There was a problem hiding this comment.
The function name is now confusing. it returns true in the case of "BAS", "hybrid" and "PE". the previous name was more meaningful
| const devspaceInfo = await devspace.getDevspaceInfo(); | ||
| const projects = await getProjectsInfo(basToolkitAPI); |
There was a problem hiding this comment.
these two calls are independent of each other, consider calling at the same time, e.g. Promise.all (...)
| return runPlatform; | ||
| } | ||
|
|
||
| export async function reportProjectTypesToUsageAnalytics( |
There was a problem hiding this comment.
missing the function return type
| ) { | ||
| const devspaceInfo = await devspace.getDevspaceInfo(); | ||
| const projects = await getProjectsInfo(basToolkitAPI); | ||
| void AnalyticsWrapper.traceProjectTypesStatus( |
There was a problem hiding this comment.
whether we need to report the empty data ?
There was a problem hiding this comment.
it will report only for the created projects
| ); | ||
| } | ||
|
|
||
| async function getProjectsInfo(basToolkitAPI: BasToolkit) { |
There was a problem hiding this comment.
return type is missing in function signature
| async function getProjectsInfo(basToolkitAPI: BasToolkit) { | ||
| try { | ||
| const workspaceAPI = basToolkitAPI.workspaceAPI; | ||
| const homedirProjects: string = joinPath(homedir(), "projects"); |
There was a problem hiding this comment.
consider to use 'workspace.workspaceFolders' instead of hardcode heuristics
| undefined, | ||
| homedirProjects | ||
| ); | ||
| const projectInfoList: (ProjectData | undefined)[] = await Promise.all( |
There was a problem hiding this comment.
please verify (ProjectData | undefined)[] casting.
| let projectType; | ||
| switch (type) { | ||
| case "com.sap.cap": | ||
| projectType = "CAP"; | ||
| break; | ||
| case "com.sap.ui": | ||
| projectType = "UI5"; | ||
| break; | ||
| case "com.sap.mdk": | ||
| projectType = "MDK"; | ||
| break; | ||
| case "com.sap.fe": | ||
| projectType = "FE"; | ||
| break; | ||
| case "com.sap.hana": | ||
| projectType = "HANA"; | ||
| break; | ||
| case "com.sap.lcap": | ||
| projectType = "LCAP"; | ||
| break; | ||
| default: | ||
| projectType = "BASEmpty"; |
There was a problem hiding this comment.
why is this transformation needed? why not report the actual type received?
|
Hi, Before merging this, please validate with Xin:
|
| ): void { | ||
| try { | ||
| const eventName = AnalyticsWrapper.EVENT_TYPES.PROJECT_TYPES_STATUS; | ||
| for (const type in projects) { |
There was a problem hiding this comment.
You can use typeName instead of type - it is the display name of the type and remove the for and switch case-
https://github.tools.sap/LCAP/project/blob/main/packages/artifact-management-base/src/project/ProjectData.ts#L12 .
NetaMolcho
changed the title
4 participants
No description provided.