Feature/448 refactoring type naming, extension fetcher 함수 dependencies 제거 #577

DaYoung-woo · 2024-08-01T10:26:05Z

Related issue

#488

Result

extension.js에서 fecter 관련 함수를 webview-loader.ts 파일로 옮겨 dependencies 제거하였습니다.

Work list

모호했던 GithruFetcherMap 타입 제거
extension fetcher 함수 dependencies 제거

Discussion

.
힘수의 매개변수로 await을 사용하는게 좋은 방법이 아닌 것 같습니다.
적극적인 피드백을 환영합니다!!!!!!

Co-authored-by: DaYoung-woo <iamwooda0@gmail.com> Co-authored-by: yuiseo <24oesiu@gmail.com>

Co-authored-by: yuiseo <24oesiu@gmail.com>

Co-authored-by: DaYoung-woo <iamwooda0@gmail.com>

DaYoung-woo · 2024-08-01T11:06:24Z

.
클래스 변수로 gitPath를 선언해주고 gitPath가 필요한 메소드를 사용하기 전에 setPath를 통해 세팅하였습니다.
그리고 gitPath가 필요한 메소드에서 세팅된 gitPath를 사용할 수 있도록 코드를 변경해두었습니다.

Co-authored-by: DaYoung-woo <iamwooda0@gmail.com>

ytaek

리팩토링하느라 고생하셨습니다 👍👍👍👍

다음 PR에는 webview-loader쪽 리팩토링 기대하겠습니다! 😃

ytaek · 2024-08-01T17:37:02Z

packages/vscode/src/extension.ts

-        fetchBranches,
-        fetchCurrentBranch,
-      });
+      const webLoader = new WebviewLoader(extensionPath, context, githubToken);


오오오오 드디어 extension.ts 가 멀끔해지는 날이 오는건가요?!!!

ytaek · 2024-08-01T17:38:28Z

packages/vscode/src/webview-loader.ts

+  private getCurrentWorkspacePath() {
+    const currentWorkspaceUri = vscode.workspace.workspaceFolders?.[0].uri;
+    if (!currentWorkspaceUri) {
+      throw new WorkspacePathUndefinedError("Cannot find current workspace path");
+    }
+    return normalizeFsPath(currentWorkspaceUri.fsPath);
+  }
+
+  private async setGitPath() {
+    this.gitPath = (await findGit()).path
+  }
+
+  private async fetchBranches() {
+    try {
+      return await getBranches(this.gitPath, this.getCurrentWorkspacePath());
+    } catch (e) {
+      console.debug(e);
+    }
+  }
+
+  private async fetchCurrentBranch() {
+    let branchName;
+    try {
+      branchName = await getCurrentBranchName(this.gitPath, this.getCurrentWorkspacePath());
+    } catch (error) {
+      console.error(error);
+    }
+
+    if (!branchName) {
+      const branchList = (await this.fetchBranches())?.branchList;
+      branchName = getDefaultBranchName(branchList || []);
+    }
+    return branchName;
+  }
+
+  private async fetchClusterNodes(baseBranchName?: string) {
+    const currentWorkspaceUri = vscode.workspace.workspaceFolders?.[0].uri;
+    if (!currentWorkspaceUri) {
+      throw new WorkspacePathUndefinedError("Cannot find current workspace path");
+    }
+
+    if (!baseBranchName) {
+      baseBranchName = await this.fetchCurrentBranch();
+    }
+    const gitLog = await getGitLog(this.gitPath, this.getCurrentWorkspacePath());
+    const gitConfig = await getGitConfig(this.gitPath, this.getCurrentWorkspacePath(), "origin");
+    const { owner, repo } = getRepo(gitConfig);
+    const engine = new AnalysisEngine({
+      isDebugMode: true,
+      gitLog,
+      owner,
+      repo,
+      auth: this.githubToken,
+      baseBranchName,
+    });
+
+    const { isPRSuccess, csmDict } = await engine.analyzeGit();
+    if (isPRSuccess) console.log("crawling PR failed");
+
+    return mapClusterNodesFrom(csmDict);
+  }
+


extension.ts이 가벼워진 대신에,
webview-loader가 무거워졌네요 🥹🥹🥹

webview loader쪽도
engine을 실행하는 부분, vscode와의 interface를 선언하는 부분, webview를 만들어서 react view app을 띄우는 부분 등으로
관심사 분리를 해놓는게 필요할 것 같습니다.

@DaYoung-woo 님, 해당 내용 새로 이슈로 등록하시고, 추후에 진행해보셔도 좋을 것 같습니다 : )

(알고 있으실 것 같지만) 여기에 reference in new issue를 누르면 요 내용이 그대로 이슈로 만들어집니다!

안녕하세용 멘토님!
사실 코드를 변경하면서 webview-loader.ts 파일이 너무 길어져서 이게 맞는지 의아했습니다.
멘토님께 피드백 받고 다시 고쳐나가고자 풀리퀘스트 넣었는데 적극적인 피드백 감사드립니다 🥰
피드백을 기반으로 다시 코드를 정리해보도록 하겠습니다!!
감사합니다🙇‍♀️

넵 conflict나는거 해결해서 일단 push 해서 merge 하시고,
다음 번에 webview-loader쪽 refactoring 가시지요 : )

헷갈리는 부분이 있으면 discord에서 추가 논의 하셔도 좋을 것 같습니다!

choisohyun

수고하셨습니다! 👍
코드 전체를 이해하진 못했는데 코드 스타일 관련해서 리뷰 드려보았어요! 참고만 해주시면 좋을 것 같습니다 🙇

choisohyun · 2024-08-03T10:08:04Z

packages/vscode/src/webview-loader.ts

+function normalizeFsPath(fsPath: string) {
+  return fsPath.replace(/\\/g, "/");
+}


webview loader에서만 쓰이긴 하지만 유틸성 함수로 보여서 utils 하위로 넣어줘도 괜찮을 것 같네요!

@DaYoung-woo 님이 만드신 부분은 아니긴 하지만, 참고로 올려봅니다.

https://nodejs.org/api/path.html#pathnormalizepath
대부분 언어의 기본 lib에는 OS path separator에 대한 설정이 들어있긴 합니다.
node도 path 쪽에서 sep 이라는 변수를 가지고 있으니,
이런 부분 활용하는 방향으로 추후 수정해도 괜찮겠네요 :)

소현님의 피드백대로 utils 하위로 넣어두겠습니당:D

const path = require('path');
function normalizeFsPath(fsPath) {
return fsPath.replace(path.sep, "/");
}

이렇게 변경하고 테스트해보겠습니다

choisohyun · 2024-08-03T10:11:38Z

packages/vscode/src/webview-loader.ts

+    let branchName;
+    try {
+      branchName = await getCurrentBranchName(this.gitPath, this.getCurrentWorkspacePath());
+    } catch (error) {
+      console.error(error);
+    }
+
+    if (!branchName) {
+      const branchList = (await this.fetchBranches())?.branchList;
+      branchName = getDefaultBranchName(branchList || []);
+    }
+    return branchName;


Suggested change

let branchName;

try {

branchName = await getCurrentBranchName(this.gitPath, this.getCurrentWorkspacePath());

} catch (error) {

console.error(error);

}

if (!branchName) {

const branchList = (await this.fetchBranches())?.branchList;

branchName = getDefaultBranchName(branchList || []);

}

return branchName;

try {

const currentBranchName = await getCurrentBranchName(this.gitPath, this.getCurrentWorkspacePath());

if (!currentBranchName) {

const branchList = (await this.fetchBranches())?.branchList;

return getDefaultBranchName(branchList || []);

}

return currentBranchName;

} catch (error) {

console.error(error);

}

형식 차이긴 한데 const만 사용하려고 하면 이렇게 쓸수도 있겠네요!

choisohyun · 2024-08-03T10:13:49Z

packages/vscode/src/webview-loader.ts

+    });
+
+    const { isPRSuccess, csmDict } = await engine.analyzeGit();
+    if (isPRSuccess) console.log("crawling PR failed");


여기는 isPRSuccess인데 crawling PR failed인가요? 👀

우와 매의 눈이십니다 👍

엇 그러네용!! 이부분도 수정하겠습니당!!! 꼼꼼한 코드리뷰 감사합니당👍

ytaek

LGTM 입니다!!

choisohyun

🚀

Revert(vscode): vscode 모듈 #577 이전으로 롤백

yuiseo and others added 2 commits July 30, 2024 23:45

Refactor: githru#448 type naming, extension fetcher 함수 dependencies 제거

483c763

Co-authored-by: DaYoung-woo <iamwooda0@gmail.com> Co-authored-by: yuiseo <24oesiu@gmail.com>

불필요한 주석 제거

46a8c51

Co-authored-by: yuiseo <24oesiu@gmail.com>

DaYoung-woo added 💻 vscode discussion question labels Aug 1, 2024

DaYoung-woo assigned yuiseo and DaYoung-woo Aug 1, 2024

DaYoung-woo requested review from a team as code owners August 1, 2024 10:26

Refactor: githru#448 parameter에서 gitpath관련 await 제거

40798e1

Co-authored-by: DaYoung-woo <iamwooda0@gmail.com>

Remove: 불필요한 콘솔 출력 제거

54d18eb

Co-authored-by: DaYoung-woo <iamwooda0@gmail.com>

ytaek previously approved these changes Aug 1, 2024

View reviewed changes

choisohyun previously approved these changes Aug 3, 2024

View reviewed changes

Merge branch 'main' into feature/448

fec3c6e

DaYoung-woo dismissed stale reviews from choisohyun and ytaek via fec3c6e August 4, 2024 09:35

[fix]ㅁ 오타 수정

8087514

DaYoung-woo mentioned this pull request Aug 4, 2024

[vscode] webview-loader 코드 분리 #599

Closed

ytaek approved these changes Aug 4, 2024

View reviewed changes

choisohyun approved these changes Aug 4, 2024

View reviewed changes

DaYoung-woo merged commit a838853 into githru:main Aug 4, 2024
2 checks passed

ytaek added this to the v0.7.0 milestone Aug 10, 2024

yuiseo mentioned this pull request Aug 13, 2024

Revert(vscode): vscode 모듈 #577 이전으로 롤백 #643

Merged

seungineer added a commit that referenced this pull request Aug 18, 2024

Merge pull request #643 from yuiseo/main

f1c860c

Revert(vscode): vscode 모듈 #577 이전으로 롤백

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/448 refactoring type naming, extension fetcher 함수 dependencies 제거 #577

Feature/448 refactoring type naming, extension fetcher 함수 dependencies 제거 #577

DaYoung-woo commented Aug 1, 2024 •

edited

Loading

DaYoung-woo commented Aug 1, 2024 •

edited

Loading

ytaek left a comment •

edited

Loading

ytaek Aug 1, 2024

ytaek Aug 1, 2024

ytaek Aug 1, 2024

ytaek Aug 1, 2024

DaYoung-woo Aug 2, 2024 •

edited

Loading

ytaek Aug 2, 2024

choisohyun left a comment

choisohyun Aug 3, 2024

ytaek Aug 3, 2024

DaYoung-woo Aug 4, 2024

DaYoung-woo Aug 4, 2024

choisohyun Aug 3, 2024

choisohyun Aug 3, 2024

ytaek Aug 3, 2024

DaYoung-woo Aug 4, 2024

ytaek left a comment

choisohyun left a comment

Feature/448 refactoring type naming, extension fetcher 함수 dependencies 제거 #577

Feature/448 refactoring type naming, extension fetcher 함수 dependencies 제거 #577

Conversation

DaYoung-woo commented Aug 1, 2024 • edited Loading

Related issue

Result

Work list

Discussion

DaYoung-woo commented Aug 1, 2024 • edited Loading

ytaek left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaYoung-woo Aug 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

choisohyun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ytaek left a comment

Choose a reason for hiding this comment

choisohyun left a comment

Choose a reason for hiding this comment

DaYoung-woo commented Aug 1, 2024 •

edited

Loading

DaYoung-woo commented Aug 1, 2024 •

edited

Loading

ytaek left a comment •

edited

Loading

DaYoung-woo Aug 2, 2024 •

edited

Loading