feat: crawler to get git PR information with detail #30

ansrlm · 2022-07-29T18:53:20Z

Remarks

제가 알기로는 Github API v3만 unauth access가 가능한 것으로 알고 있는데, token을 입력하지 않아도 octokit이 호출을 하는 것으로 확인했습니다. 아예 다른 방식을 채택하고 있는 것으로 보이며,, 해당 내용은 npm모듈 document에도 없어서 이부분은 모듈 내부를 좀 확인해야 할 것 같습니다.

Incoming keynotes

jest or mocha based unit test
sleep or wait function not to exceed limits

FYI

https://stackoverflow.com/questions/61445703/how-to-allow-a-public-app-to-connect-to-the-github-api-v4-without-an-access-toke

…call

hy57in · 2022-07-30T04:42:18Z

crawler/ansrlm/src/utils/input.util.ts

+export const isValidForOctokit = (input: string) => {
+  const regex = /^[A-Za-z0-9\-_]+\/[A-Za-z0-9\-_]+$/;
+  return regex.test(input);
+};
+
+export const getOctokitProps = (input: string) => {
+  const inputArray = input.split('/');
+  return {owner: inputArray[0], repo: inputArray[1]};
+};


util 파일 네이밍을 {filename}.util.ts 로 하는 이유가 궁금합니다!
cmd + p 로 해당 파일을 찾을 때 util인지 알기 쉽게 하기 위함인가요?

저도 비슷한 고민을 한적이 있는데,

파일이 많아질 수록 .util 이 붙으면 타이핑이 귀찮고 지저분해진다
vs
cmd + p로 찾기 편하고 구분하기 쉽다

의견 대립 때문에 고민한적이 있거든요..
어떤 이유 때문에 이런 네이밍을 선호하시는지 단순 취향이 궁금합니다!

1.말씀 주신 부분처럼 파일을 찾는 행위에 도움이 됩니다.

2.대부분의 경우 fila path를 통해서 어떤 파일행위를 할 것인지 유추가 가능합니다. import {time} from "utils/time"
그러나 특정 이유(기능 단위의 파일 관리적 측면을 한다거나,,,)로 인해서 디렉토리가 정확히 분리되어 있지 않은 경우가 발생하거나 디렉토리 내부에서 상대경로로 접근하는 경우는 file path를 통해서 유추하기가 어렵습니다.

이러한 경우를 위해서 filename.desc.ts 와 같은 패턴을 사용하고 있습니다.
사실 이러한 습관(?)은 Angular에서 지향하는 패턴과의 중간 영역에 놓여 있습니다.
Angular는 더 나아가서 기능 단위의 파일 디렉토리를 하기보다는 영역 단위의 파일 디렉토리 구성을 지향하는 것 같습니다. (/utils/ 보다는 /octokits/ 를 선호)

3.파일 테스트 커버리지를 다르게 적용할 수 있습니다. 이부분도 가정이지만, 기능 단위의 디렉토리 구조를 강하게 구성하고 있다면 pattern matching을 잘 사용하면 utils와 services에 대한 테스트 커버리지를 다르게 설정할 수 있습니다. 그러나 테스트파일들의 구성 구조가 레포 전반에 대해서 scatter 정도가 높다면, 중간에 네이밍을 넣는 것으로 **.util.spec.**와 같이 기능 단위에 대한 실행을 가능하게 할 수 있습니다.

FYI
https://v2.angular.io/docs/ts/latest/guide/style-guide.html#!#02-01

ytaek · 2022-07-30T04:47:59Z

crawler/ansrlm/src/services/octokit.service.ts

+
+  constructor(props: Props) {
+    this.props = {...props};
+    this.octokit = new Octokit();


나~~중에 싱글톤으로 만들어도 괜찮지 않을까요? :- )

넵 말씀이 맞습니다. 어제 새벽에 급하게 짜고 넣다보니까 그런 부분에 대해서 생각을 못했습니다. 😢
프로덕트를 실제 만드는 레벨에서는 말씀주신 것처럼 싱글톤으로 꺼내는 것이 정말 좋을 것 같습니다.

싱글톤으로 만드실때
인스턴스를 생성 후 생성한 인스턴스를 export하는 방법으로 구현하시나요??

그부분은 차후 작업을 진행하면서 결정지어야 할 사항일 것 같습니다.

처음 어플리케이션(모듈)이 실행될 때 무조건적으로 실행이 필요한 사항이라면 말씀하신 방향에 기반해 프로젝트의 root에서 주입을 해주는 방식으로 처리할 것 같습니다.

이와 반대로 인스턴스 생성이 특정 요건에 따라서 실행이 된다면 그부분은 인스턴스가 없으면 생성하고, 있다면 해당 인스턴스를 반환하는 방식으로 진행하는 것을 고려할 수 있겠습니다.

이에 더해서 사실 new Octokit(config?: OctokitConfig)와 같은 구조라서 내부의 config를 각각 다르게 사용해야 하는 상황이 발생한다면 이 부분을 어떻게 처리할지 재논의가 필요할수도 있겠습니다.

평범하게 class로 싼 다음에 instance()로 뺄 수도 있을 것 같고.

private _instance = new Octokit(); public get instance() ...

@ansrlm 님 말씀처럼 root에서 주입을 해줘도 될 것 같구요.

이에 더해서 사실 new Octokit(config?: OctokitConfig)와 같은 구조라서 내부의 config를 각각 다르게 사용해야 하는 상황이 발생한다면 이 부분을 어떻게 처리할지 재논의가 필요할수도 있겠습니다.

저라면 Dependency Injection Container를 쓰겠습니다.. ㅎㅎ
tsrynge정도만 써봤는데, Spring같이 풍부하지는 않지만 그럭저럭 쓸 만한 것 같습니다.

ytaek · 2022-07-30T04:48:51Z

crawler/ansrlm/src/services/octokit.service.ts

+    const {data} = await this.octokit.rest.pulls.list({
+      owner,
+      repo,
+    });
+


와우...ocktokit쓰면 엄청 편리해지네요 ㄷㄷ 안 쓸 이유가 없겠습니다.

저도 덕분에 ocktokit 란걸 알아갑니다..! 너무 좋네요!!ㅎㅎㅎㅎ
(근데 이거 octotree 랑은 아무관계없는건가요..?ㅋㅋ)

ooooorobo

LGTM! 오 octokit 편하고 좋아보이네요! 👀👍

taejs

고생하셨습니다~ 제 리뷰는 반영이 꼭 필요한것들은 아니어서 참고만 부탁드립니다.

auth 관련해서는 조금 찾아봤는데 아직 확실한건 아니지만 github enterprise가 아니라 github 레포라서 인증 없어도 가져와지는것 같습니다.
octokit - 인증

taejs · 2022-07-30T13:20:50Z

crawler/ansrlm/src/index.ts

+
+  const isDetail = process.argv[3];
+
+  const result = await octokitService.getPullRequests(!!isDetail && isDetail === 'detail');


만약에 --detail 처럼 옵션을 받도록 하면 process.argv.includes('--detail') 이런식으로도 변경이 가능할것 같습니다!

이 pr목적이랑은 크게 관련이 없기도 하고 적용해도 큰 차이는 없어서 지금 형태도 좋습니다.

말씀처럼 --detail을 argv에서 찾는 방식도 좋을 것 같습니다. ㅎㅎ

start: cd dist && node index.js 로 구성을 했는데요, 이부분을 차후에는 다음과 같은 셸 스크립트로 구성할 생각도 가지고 있습니다.
node -e require('index.js').execute({detail: $1, input: $2})

변경 작업을 할 때 한번 더 논의 요청드리도록 하겠습니다. 감사합니다.

taejs · 2022-07-30T13:22:31Z

crawler/ansrlm/src/services/octokit.service.ts

+      const pullNumbers: number[] = [];
+      for (const pullRequest of data) {
+        pullNumbers.push(pullRequest.number);
+      }


data.map((pr=> pr.number)) 처럼 map을 사용할수도 있을것 같습니다!

~~안타깝게도 data가 Array의 형태가 아니라서 Iterator가 존재하지 않는 상황이었습니다.~~

엥..? 지금 보니까 of로 처리를 하고 있네요? 잠에 취해서 착각을 했었나 봅니다. 😢
이부분은 말씀주신 방법처럼 다시 변경하도록 하겠습니다.

해결된 얘기에 말꼬투리를 잡는거 같아서 찝찝하지만,,

of로 처리하는건 이터러블 프로토콜을 따른다는건데,
이터러블 프로토콜을 따른다고 해서 배열의 메서드인 map을 사용할 수 있다는 건 잘못된 얘기가 아닌가 싶어서요..

혹시 제가 잘못 이해했다면 지적부탁드립니다..!

저도 이터러블 프로토콜을 따른다고해서 항상 Array.prototype.map 을 사용할수있는건 아닌걸로 알고있습니다.

위 의견에 보충을 하자면...

const genSquare = function* (max) { let cursor = 0; while (cursor < max) { yield cursor * cursor; cursor++; } }; const nums = genSquare(5); for (const n of nums) { console.log(n); } // 0 // 1 // 4 // 9 // 16 const nums2 = genSquare(5); const nums3 = nums2.map((n) => n*100) // Uncaught TypeError: nums2.map is not a function

위 예제에서는 genSquare 를 통해 이터레이터를 만드는데요.
이터러블 프로토콜을 따르기때문에 for of 나 Spread Operator 를 사용할수는 있지만,
Array.prototype.map 을 가지고있지는 않습니다.

하지만 여기 코드리뷰 부분 for (const pullRequest of data) { ... } 에서는

data 가 const {data} = await this.octokit.rest.pulls.list( ... ) 를 통해서 받아온걸 알고있고,

이 octokit.rest.pulls.list 는 https://docs.github.com/en/rest/pulls/pulls#list-pull-requests 스펙대로 응답을 반환할것임을 알고있기때문에,

이 응답값은 array.prototype.* 을 가지고 있을거라고 생각할수도 있을 것 같습니다.

좋은 말씀 감사합니다.

자기변호적 주장으로 들릴 수 있겠으나,,, 어디서(?) 부터 잘못이 시작되었는가를 말씀드리면

1.처음에 data를 콘솔로 뽑아보고 타입을 역으로 유추했어요. 사실 그러면 안되는 것이 맞는데 @types/octokit이 바로 알아볼 수 없게 구성이 되어있다라고 할까요...

2.데이터가 Array가 아니라 Object인걸로 잘못 확인했고 보통 네트워크를 타고 오는 경우 일반객체가 대부분이고 이터레이터가 없을테니까 in 으로 재가공 처리를 하면 되겠구나 했는데요, 머리와 다르게 손은 of로 잘 사용을 하고 있었네요. (그래서 현재 적용된 부분도 나쁘진 않아 보입니다.)

3.그럼 어디부터 잘못된걸까 역으로 확인해 볼 때 "이터러블 프로토콜을 따르도록 추가 처리를 해줄 정도면 그냥 Array를 주지 않았을까" 라는 생각이 들더라구요. 역시... Array타입이 맞았습니다.

코멘트 주신 부분처럼 이터러블 프로토콜을 따르는 Object의 경우 map function을 바로 사용할 수 있는 건 아닙니다.
다만 [...object].map로 간단하게 사용은 가능할 것 같네요. 해당 적용은 객체 길이 만큼 두번 도는 이슈가 있긴 하겠습니다.

참고로 octokit.rest.pulls.list 의 반환값에 대한 정의를 따라가보면... (좀 복잡합니다 ㅠㅠ)

list: { (params?: RestEndpointMethodTypes["pulls"]["list"]["parameters"]): Promise<RestEndpointMethodTypes["pulls"]["list"]["response"]>; defaults: RequestInterface["defaults"]; endpoint: EndpointInterface<{ url: string; }>; }

=> Promise<RestEndpointMethodTypes["pulls"]["list"]["response"]>; 를 따라가보면

list: { parameters: RequestParameters & Omit<Endpoints["GET /repos/{owner}/{repo}/pulls"]["parameters"], "baseUrl" | "headers" | "mediaType">; response: Endpoints["GET /repos/{owner}/{repo}/pulls"]["response"]; };

=> Endpoints["GET /repos/{owner}/{repo}/pulls"]["response"]; 를 따라가보면

/** * @see https://docs.github.com/rest/reference/pulls#list-pull-requests */ "GET /repos/{owner}/{repo}/pulls": Operation<"/repos/{owner}/{repo}/pulls", "get">;

=> Operation<"/repos/{owner}/{repo}/pulls", "get">; 를 따라가보면

declare type Operation<Url extends keyof paths, Method extends keyof paths[Url], preview = unknown> = { parameters: ToOctokitParameters<paths[Url][Method]> & RequiredPreview<preview>; request: { method: Method extends keyof MethodsMap ? MethodsMap[Method] : never; url: Url; headers: RequestHeaders; request: RequestRequestOptions; }; response: ExtractOctokitResponse<paths[Url][Method]>; };

=> ExtractOctokitResponse<paths[Url][Method]>; 를 따라가보면

declare type ExtractOctokitResponse<R> = "responses" extends keyof R ? SuccessResponseDataType<R["responses"]> extends never ? RedirectResponseDataType<R["responses"]> extends never ? EmptyResponseDataType<R["responses"]> : RedirectResponseDataType<R["responses"]> : SuccessResponseDataType<R["responses"]> : unknown;

=> SuccessResponseDataType<R["responses"]> 를 따라가보면

declare type SuccessResponseDataType<Responses> = { [K in SuccessStatuses & keyof Responses]: GetContentKeyIfPresent<Responses[K]> extends never ? never : OctokitResponse<GetContentKeyIfPresent<Responses[K]>, K>; }[SuccessStatuses & keyof Responses];

=> OctokitResponse<GetContentKeyIfPresent<Responses[K]>, K>; 를 따라가보면

export declare type OctokitResponse<T, S extends number = number> = { headers: ResponseHeaders; status: S; url: Url; data: T; };

=> data: T; 에 바인딩되는 타입을 찾아보면

"/repos/{owner}/{repo}/pulls": { // ... get: operations["pulls/list"]; post: operations["pulls/create"]; };

=> get: operations["pulls/list"]; 를 따라가보면

"pulls/get": { // ... responses: { 200: { content: { "application/json": components["schemas"]["pull-request-simple"][]; }; }; // ... }; };

=> components["schemas"]["pull-request"] 를 따라가보면

export interface operations { // ... "pull-request": { state?: "open" | "closed" | "all" | undefined; head?: string | undefined; base?: string | undefined; sort?: "created" | ... 3 more ... | undefined; direction?: "asc" | ... 1 more ... | undefined; per_page?: number | undefined; page?: number | undefined; } }

=> 드디어 나왔습니다

정리하면,
octokit.rest.pulls.list( ... ) 은 components["schemas"]["pull-request-simple"][]; 를 반환하도록 정의되어 있으므로
Array.prototype 을 가지고 있다고 볼수있겠습니다.

@anpaul0615

저도 역으로 찾아서 한번 드릴까해서 파일을 뒤적거리고 있었는데요, 도움 감사드립니다 (_ _)

앗 같은부분을 보고 계셨군요 ㅎㅎ
저도 윗 의견들 보고 급 궁금해서 찾아들어가다보니 octokit 타입이 이리저리 꼬여있더라구요;;;;

1.처음에 data를 콘솔로 뽑아보고 타입을 역으로 유추했어요. 사실 그러면 안되는 것이 맞는데 @types/octokit이 바로 알아볼 수 없게 구성이 되어있다라고 할까요...

찾고보니 이 말씀이 매우매우매우 격하게 공감이 됐습니다 ㅠㅠ

앗 작성한게 뭔가 날라갔네요,,

우선, 표현에 대하여 트집(?) 잡는 느낌의 review를 작성한거 같은데 좋게 받아주셔서 감사합니다!
덕분에 @ansrlm 님의 사고(?)를 간접 체험할 수 있어 좋았습니다 ㅎㅎ
@anpaul0615 님 덕분에 타입 추론을 하는 방법을 체득한것 같습니다!!

두분다 성실한 답변 감사드립니다.

taejs · 2022-07-30T13:23:39Z

crawler/ansrlm/src/utils/input.util.ts

+  const inputArray = input.split('/');
+  return {owner: inputArray[0], repo: inputArray[1]};


const [owner, repo] = input.split('/');
return {owner, repo}
로 축약도 가능할것 같습니다!

넵 말씀 감사합니다~
다음 PR 생성에 해당 부분을 리팩토링해서 같이 넣어야겠습니다.

ytaek · 2022-07-30T17:47:50Z

auth 관련해서는 조금 찾아봤는데 아직 확실한건 아니지만 github enterprise가 아니라 github 레포라서 인증 없어도 가져와지는것 같습니다. octokit - 인증

public repo이면 인증이 없어도 뭔가 되나보군요!!
대신 access limit이 걸려있겠죠? ㅜ.ㅜ

feat: crawler to get git PR information with detail

22c03a5

ansrlm requested review from wherehows, ooooorobo, blcklamb, rachel490, hy57in, kmin-jeong, jejecrunch, alledy, anpaul0615, jin-Pro, hyeda1103, ag502, hanseul-lee, momomingzhi, vgihan, pshdev1030, seyoung8239, jeonghye-choi, taejs and ytaek as code owners July 29, 2022 18:53

ansrlm self-assigned this Jul 29, 2022

fix: treat as non-blocking function call and encompass same function …

9734743

…call

hy57in reviewed Jul 30, 2022

View reviewed changes

ytaek approved these changes Jul 30, 2022

View reviewed changes

ooooorobo approved these changes Jul 30, 2022

View reviewed changes

taejs approved these changes Jul 30, 2022

View reviewed changes

ansrlm merged commit eb317ed into githru:main Aug 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: crawler to get git PR information with detail #30

feat: crawler to get git PR information with detail #30

ansrlm commented Jul 29, 2022 •

edited

Loading

hy57in Jul 30, 2022

ansrlm Jul 30, 2022

ytaek Jul 30, 2022

ansrlm Jul 30, 2022

jin-Pro Jul 30, 2022

ansrlm Jul 30, 2022

ytaek Jul 30, 2022

ytaek Jul 30, 2022

anpaul0615 Jul 30, 2022

ooooorobo left a comment

taejs left a comment

taejs Jul 30, 2022

ansrlm Jul 30, 2022

taejs Jul 30, 2022

ansrlm Jul 30, 2022 •

edited

Loading

jin-Pro Jul 31, 2022

anpaul0615 Jul 31, 2022

ansrlm Jul 31, 2022

anpaul0615 Jul 31, 2022

ansrlm Jul 31, 2022

anpaul0615 Jul 31, 2022

jin-Pro Jul 31, 2022

taejs Jul 30, 2022

ansrlm Jul 30, 2022

ytaek commented Jul 30, 2022


		const isDetail = process.argv[3];

		const result = await octokitService.getPullRequests(!!isDetail && isDetail === 'detail');

		const inputArray = input.split('/');
		return {owner: inputArray[0], repo: inputArray[1]};

feat: crawler to get git PR information with detail #30

feat: crawler to get git PR information with detail #30

Conversation

ansrlm commented Jul 29, 2022 • edited Loading

Contents

Remarks

Incoming keynotes

FYI

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

ooooorobo left a comment

Choose a reason for hiding this comment

taejs 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

ansrlm Jul 30, 2022 • 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

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 commented Jul 30, 2022

ansrlm commented Jul 29, 2022 •

edited

Loading

ansrlm Jul 30, 2022 •

edited

Loading