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.

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

feat(vscode): Add private method 'ensureOctokitInstance' to resolve Octokit's common logic. #562

Merged
merged 4 commits into from
Aug 1, 2024

Conversation

bbanderson
Copy link
Contributor

Related issue

#561

Result

Octokit 인스턴스가 이미 있으면 바로 가져오고, 없다면 새로 만들어서 가져오는 로직이 중복되고 있었습니다.
그래서 그 부분을 private method로 따로 만들었습니다.
이 메서드가 호출되면, Octokit이 존재함이 보장됩니다.

Work list

스크린샷 2024-07-26 오후 10 11 07

Discussion

더 좋은 방법 있다면 말씀 부탁드립니다~

choisohyun
choisohyun previously approved these changes Jul 27, 2024
Copy link
Contributor

@choisohyun choisohyun left a comment

Choose a reason for hiding this comment

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

중복코드 개선 넘 좋은 것 같습니다! 👍

packages/vscode/src/credentials.ts Show resolved Hide resolved
packages/vscode/src/credentials.ts Show resolved Hide resolved
packages/vscode/src/credentials.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@mdgarden mdgarden left a comment

Choose a reason for hiding this comment

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

코드 개선 넘 좋습니다👍👍👍

packages/vscode/src/credentials.ts Show resolved Hide resolved
choisohyun
choisohyun previously approved these changes Jul 30, 2024
@ytaek
Copy link
Contributor

ytaek commented Jul 30, 2024

compile 에러 한번 확인해주시고, (type 에러 같습니당)
아마 non-null assertion 수정하다가 난 것 같은데,
일단 현재 부분 넣고, 추가적으로 개선하셔도 괜찮을 것 같습니다.
(잊지 않도록 이슈로 꼭 등록하신 다음에 연결해서 PR올려주세요!)

@bbanderson
Copy link
Contributor Author

compile 에러 한번 확인해주시고, (type 에러 같습니당) 아마 non-null assertion 수정하다가 난 것 같은데, 일단 현재 부분 넣고, 추가적으로 개선하셔도 괜찮을 것 같습니다. (잊지 않도록 이슈로 꼭 등록하신 다음에 연결해서 PR올려주세요!)

@ytaek 재확인 부탁드립니다~!

Copy link
Contributor

@ytaek ytaek left a comment

Choose a reason for hiding this comment

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

LGTM 입니다!!

packages/vscode/src/credentials.ts Show resolved Hide resolved
packages/vscode/src/extension.ts Show resolved Hide resolved
@mdgarden mdgarden merged commit 4adca1f into githru:main Aug 1, 2024
2 checks passed
@ytaek ytaek added this to the v0.7.0 milestone Aug 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants