-
Notifications
You must be signed in to change notification settings - Fork 82
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(engine): (WIP) implement STEM #73
Conversation
ㄷㄷㄷㄷㄷ 그림 너무 멋집니다. |
export interface Stem { | ||
id: string; | ||
headId: string; | ||
tailId: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- id => dictionary key로 관리되기 때문에, 필요 없음
- headId => CSM 만들 시, splice로 자른다면 필요 없을 수 있음 (일단 없애고 STEM이 재사용되는 곳이 생긴다면, 다시 추가)
- tailId => 사용하는 곳 없음, 필요 X
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이부분도 비즈니스 로직에 대한 부분이다보니
https://github.com/githru/githru-vscode-ext/pull/73/files#diff-a21f607ebb0be4d194796dadf7295e1e1806c57825e631ecb2a5bc606365f634R55 처럼 설명을 작성하는 것도 한 방법일 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- id => dictionary key로 관리되기 때문에, 필요 없음
이건 어떤 의미일까요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이건 어떤 의미일까요??
아마 StemDictionary 를 branch 명으로 접근하는 부분일 것 같습니다.
stemDict = { [branch] : stem }
구조에서
이미 branch 를 알고있는 상황에서 stem 을 찾아온건데
stem 안에 branch(=stem.id) 가 있을 필요가 없다 라는 의미로 보입니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
너무 저만 알아보게 썼네요 😂 결론은 Stem interface에서 id, headId, tailId 속성 모두 삭제하겠다는 의미입니다
- id
- 커밋들은
buildStemDict
함수를 통해 결과적으로Map<string, Stem>
형태의 딕셔너리에 <Stem.id, Stem 객체> 쌍으로 담기게 돼요. - stem.id의 용도는 stemDict에서 해당 id값을 가지는 Stem 객체를 찾는 것입니다. 이때 Stem 객체는 자신의 id를 몰라도 상관 없어요.
- 그런데 stem.id가 branch 이름을 나타내기도 해서 필요한 상황이 있을 것 같기도 하고요.. 🤔 이건 CSM 구현 방식에 따라 남길지 말지 여부가 달라질 것 같습니다.
- 커밋들은
- headId
- CSM을 만들 때, 항상 Stem의 모든 커밋 노드들을 스쿼시 머지하는 것이 아니라, 일부 노드만 스쿼시 머지될 수도 있어요.
- 일부 노드만 스쿼시 머지된 경우에, 어디까지 스쿼시 머지 시켰는지 기억하는 포인터 같은 역할이에요
- @anpaul0615 님과 논의 중 당장은 stem.nodes 배열을 splice해서 스쿼시 해도 될 것 같다고 이야기가 되었고, 따라서 headId 포인터는 당장 필요하지 않게 되었습니다.
- 만약에 stem 객체가 가지고 있는 nodes를 CSM을 만들 때 이외에도 사용한다! 라고 하면 stem.nodes를 훼손시키지 않기 위해 headId 포인터가 필요할 수도 있을 것 같아요.
- tailId
- STEM에 속하는 커밋 노드 배열에서 가장 나중에 커밋된 커밋의 아이디입니다. nodes[nodes.length-1]
- 사용하는 곳이 없어서 삭제할 속성이에요
packages/analysis-engine/src/stem.ts
Outdated
|
||
while (!q.isEmpty()) { | ||
const tail = q.pop(); | ||
// eslint-disable-next-line no-continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no-continue 규칙 풀고 싶습니다..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 라인에만 사용되는 것일까요?
만약에 해당 라인에만 필요한 것이라면 지금 처리하신 방식으로 진행하시고
전반적으로 필요하다면 현 release일정이 급한 상황이라서 우회하는 로직을 구성하기보다는
말씀주신 것처럼 no-continue에 대한 rule을 삭제해도 좋을 것 같습니다. (다른 곳은 사용하지 않으니 해당 PR에서 진행하면 좋을 것 같습니다.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 없애는거에 한표요!
continue를 못 쓰게 하는건 참 과한 것 같아요;;; ㅜ.ㅜ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그렇다면.. 요번 PR에서 no-continue 규칙을 off 시키도록 하겠습니다. 👏
const q = new Queue<CommitNode>(); | ||
q.push.bind(q); | ||
|
||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 주석 처리 차후 유지보수에 많은 도움이 될 것 같네요!
@@ -0,0 +1,8 @@ | |||
import { CommitNode } from "./CommitNode"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
금일 회의에서 협의된 index.ts에서 전체 타입들을 관리하는 부분은 모두 다 머지된 이후에 추가 PR을 통해서 진행하는 것이 좋을 것 같다는 생각이 들었습니다. (conflict를 최소화 하기 위해서)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 고생하셨습니다~!!
현 처리까지는 확인했고, 추가 수정해주시면 확인하도록 하겠습니다.
PR description 내 예제이미지와 동일한 형상으로 커밋을 만들어 봤습니다. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGGGGGGGGGGGGGTM! 테스트까지 훌륭하네요!!
커멘트 몇개 남겼습니다~.
packages/analysis-engine/src/stem.ts
Outdated
|
||
while (!q.isEmpty()) { | ||
const tail = q.pop(); | ||
// eslint-disable-next-line no-continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 없애는거에 한표요!
continue를 못 쓰게 하는건 참 과한 것 같아요;;; ㅜ.ㅜ
export interface Stem { | ||
id: string; | ||
headId: string; | ||
tailId: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- id => dictionary key로 관리되기 때문에, 필요 없음
이건 어떤 의미일까요??
import { CommitNode } from "./types/CommitNode"; | ||
import { CommitRaw } from "./types/CommitRaw"; | ||
|
||
type CommitDict = Map<string, CommitNode>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(궁금) Map을 사용하신 특별한 이유가 있을까요? - ordered 때문이라던지?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분은 나중에 serialize 문제때문이라도 한번 생각해볼 필요가 있겠네요.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map#objects_vs._maps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ytaek commit.id로 CommitNode를 바로 찾기 위해 사용했습니다.
여기에 따로 타입을 선언한 이유는, util의 두 함수에서 공통으로 commitDict를 사용하는데(L6,L15
),
Map<string, CommitNode>
을 반복해서 쓰는 것보다는 CommitDict 타입이 따로 있는 게 알아보기 좀 더 편할 것 같아서였어요 🙂
@anpaul0615 제가 serialize 문제 관련해서 아직 잘 이해를 못했는데, Map 객체를 Stem이나 CSM을 만들기 위해 engine 내부에서만 사용해도 문제가 될까요? 저는 CommitDict는 엔진 안에서만 사용하고 뷰로는 전달되지 않는다고 생각했어요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ooooorobo 아마 @ytaek 님 질문은 (ordered 언급을 하신걸로보아) Map vs Object 인 것 같아요. 혹시 이 부분도 말씀해주실수있나요?
@ooooorobo serialize 에 대해서는 조금 더 생각을 정리한다음에 말씀드릴게요. 아직 정리가 안되서 질문드리기가 좀 어렵네요..ㅠ
return new Map( | ||
commits.map( | ||
(commit) => [commit.id, { traversed: false, commit } as CommitNode], | ||
[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(궁금2) Map 생성자로 저렇게 넣는 방식은 처음봅니다!!! 씐기하네요! ㅎㅎ
혹시 괜찮으시면 설명 좀 부탁드려요!!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 신기해서 찾아봤는데 Map 의 constructor 에서 iterable 을 지원하네요!
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map/Map
덕분에 하나 배워갑니다ㅎㅎㅎ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
링크 첨부 감사합니다! 👍 mdn을 약간 긁어와 요약하자면
const myMap = new Map([
[1, 'one'],
[2, 'two'],
[3, 'three'],
]);
요런 식으로, [key, value] 쌍들을 배열로 감싸서 Map constructor에 넣어주면 Map 객체에 값을 바로 채워줄 수 있습니다
|
||
export function getLeafNodes(commitDict: CommitDict): CommitNode[] { | ||
const leafs: CommitNode[] = []; | ||
commitDict.forEach((node) => node.commit.branches.length && leafs.push(node)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ooooorobo 님 코드가 짧고 간결해서 매우 좋은 것 같습니다. :-)
(chore) 가독성 등을 생각하면 어차피 stream 류(?)의 function들을 쓴다면
.filter( ... ).forEach(..)
로 해도 괜찮을 것 같습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 이부분은 forEach + inner filter 보다, filter + forEach 가 더 읽기 좋은것 같습니다 ㅎㅎ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(매우 궁금) commitDict의 크기가 어느정도일지는 모르겠으나, filter + forEach의 코드는 O(2*N)이라는 시간 소모가 소비될거라 생각하는데, 가독성이 중요시 되는게 우선이어야 하는건가요???
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(잡설)
지극히 개인적으로는 대부분의 변화에는 tradeoff 가 있다고 생각하는 편인데용.
O(2*N) vs 가독성으로 따져봐야겠네요 ㅎㅎ
O(2*N)이지만, iteration하는 비용이 별로 크지 않다거나, N이 몇천~만 넘어가는 케이스가 아니라면 큰 차이는 없을 수도 있겠구요.
(사실 N^2 이런거 아니고 상수정도 차이나는건 보통은 critical 하지 않을 수도 있으니까요.)
튜닝할 때도 팔레토 법칙 같은게 잘 쓰일 수 있겠는데요.
저런 작은 코드 튜닝하는데 리소스를 쓰는것 보다,
실제 러닝타임에서 병목지점이 되는 지점을 잡아서 해당 부분을 (80vs20에서 20을) 타겟팅해서 시간을 줄일 수 있다면 그게 더 좋을 수도 있다는 거죵 ㅎㅎ
그래서 옛날옛적 웹사이트 할때는... 코드를 어떻게든 돌아가게만 짜고, 나머지는 DB Query Tuning 전문가를 불러 성능을 해결하는 케이스도 있었죵... ㅎㅎ
가독성의 비용은 정말 측정하기 어렵긴한데,
O(2*N)의 비용이 그리 크지 않다고 생각되면 가독성을 위주로 가는 것도 좋은 것 같아요.
그리고 (위 케이스를 말하는 건 아니고) 가독성이 좋고 만약 더 단순하게 짤 수 있다면, 추후에 optimizer한테 책임을 다 넘겨버릴 수 있으니까요.
나중에 @jin-Pro 님이 요 코드 가지고 러닝 타임 테스트 한번 해주시죠 : -)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ytaek 답변 감사드립니다 ㅎㅎ 이렇게 또 하나 배워갑니다!
O(2N) 의 시간복잡도에서 N이 몇 천 ~ 만 이 넘어가지 않을 거라는 점과 O(2N)과 O(N)은 critical 하지 않다는 부분 !
때문에 가독성을 가지고 가는 것도 좋은 판단일 수 있다는 점 유의하겠습니다 ㅎㅎ
다음에 기회되면 러닝 타임 테스트 진행해보겠습니다!
생각을 공유해주셔서 감사드립니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
의견 감사합니다! .filter.forEach
로 나누면 이해하기 더 편할 것 같아요. 👍
그런데 슬프게도 commitDict를 Map 객체로 받아서.. 😓 forEach 밖에 쓸 수가 없었어요. filter는 Array.prototype
함수이기 때문에 ..
filter를 쓰려면
[...commitDict].filter(...)
이렇게 spread 시켜줘야 하는데, 가독성을 위해서 배열로 변환까지 하는 것보다는 이해를 돕는 주석을 하나 추가하는게 좋지 않을까 하는 생각이 들어요.
(+) getLeafNodes
에서 commitDict Map 객체를 받고 있는 이유는, 이 util 함수는 buildStem
함수에서 쓰기 위해 만들어진 함수인데, buildStem
에서는 커밋 배열이 아니라 commitDict Map 객체만을 파라미터로 받고 있기 때문입니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그런데 슬프게도 commitDict를 Map 객체로 받아서.. 😓 forEach 밖에 쓸 수가 없었어요. filter는 Array.prototype 함수이기 때문에 ..
이래서.. js는 좀 짬뽕인것 같습니다. ㅋ library의 통일성 같은게 별로 없는 느낌 ㅜ.ㅜ
API 문서를 항상 끼고 살아야 되는 것 같아요 ㅎㅎ
그나저나 Map이 spread 되는건 오늘 또 처음 알았네요!
iterable하고 spread까지!! 또 배워갑니다 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이러한 통일성 없는 배열 이슈로 이터러블 프로토콜을 활용한 유인동님의 함수형 프로그래밍 강의가 있더라구요.
우와의 연속인 강의라 관심있으시면 보시는것도 추천드릴게요 !!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
우와 저런것 좋네요!! 👍 참고하겠습니다!!
개인적으로는 태생이 FP 인 랭귀지로 FP를 하고 싶긴 합니다 ㅋㅋ
굳이 이런 열악(?)한 js 태생에 매여서 (거기에 ts까정;;) FP를 하는건 배꼽이 더 큰 느낌이라고나 할까요;; 😸
자료는 재밌겠네용. 책도 있는 것 같은데 담에 한번 사서 봐야겠어요!
http://www.kyobobook.co.kr/product/detailViewKor.laf?mallGb=KOR&ejkGb=KOR&barcode=9788966262120
packages/analysis-engine/src/stem.ts
Outdated
function generateStem(stemNodes: CommitNode[]): Stem { | ||
const tail = stemNodes[0]; | ||
return { | ||
id: tail?.stemId ?? tail?.commit.id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아, stem의 id = 끝의 id 인가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이건 함수 파라미터로 stemId를 따로 받아야 할 것 같아요.
stem id는
tail.branches[0]
implicit-{implicitCount}
두 가지 경우로만 결정될 것 같습니다
packages/analysis-engine/src/stem.ts
Outdated
|
||
export default stem; | ||
function generateStem(stemNodes: CommitNode[]): Stem { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(사소) 변수명이 약간 헷갈리는 것 같습니다.
아래쪽부터 봤을 때는 StemNode라는 node type이 별도로 있는 줄 알았네요.
(ide로 보면 헷갈릴 일은 없을 것 같긴 합니다만;;)
stem에 들어갈 commit node
라는 뜻을 가지면 제일 좋긴 하겠네요 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 이렇게 보니 헷갈리네요 😅 속성명과 통일시켜서 그냥 nodes
라고 해줘도 괜찮을 것 같기도 해요.
packages/analysis-engine/src/stem.ts
Outdated
q: Queue<CommitNode>, | ||
implicitBranchCount: number | ||
): CommitNode[] { | ||
let now: CommitNode | undefined = commitDict.get(tailId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
undefined를 type에 넣지 않고, 먼저 체크한 다음 early return [] 으로 나가는 건 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guard Clauses 참고자료 덧붙입니다! (알고계실수도있지만ㅎㅎ)
https://refactoring.com/catalog/replaceNestedConditionalWithGuardClauses.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 Guard Clauses라고 부르는군요! 감사합니다, 반영할게요 👍
packages/analysis-engine/src/stem.ts
Outdated
tailId: string, | ||
commitDict: Map<string, CommitNode>, | ||
q: Queue<CommitNode>, | ||
implicitBranchCount: number |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
혹시 implicitBranchCount
는 어떤 용도로 사용되는지요?
count가 아니라 혹시 id나 number의 용도로 쓰인것인지 잘 몰라서 여쭤봅니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아마 N개의 implicitBranch 에 대해서 넘버링 할당하신 것 같아요.
말씀하신대로 이름이 count 가 적절하지 않을수도 있겠네요ㅎㅎ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
implicitBranch 넘버링 용도입니다!
전 당장은 implicitBranchNumber가 좀 더 나아 보이는데, 생각나는 다른 이름이 있으시다면 제안 부탁드려요 👍
const stemDict = buildStemDict(commitDict); | ||
expect(stemDict).toBeInstanceOf(Map); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function 별로 test case 가 다 있는 것 정말 좋습니다!!
여기서는 fake data 값 하나 정도 Map 에 잘 들어가는지 확인하는 것도 좋을 것 같아요!
우와 그림 직접 그린건가요? ㄷ ㄸ 대박 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
뭔가 @ooooorobo 님이 바쁘신것같아서 저도 같이 리뷰+리뷰답변 진행해봤습니다.
여러 방면으로 리뷰 남겨주셔서 감사합니다!
import { CommitNode } from "./types/CommitNode"; | ||
import { CommitRaw } from "./types/CommitRaw"; | ||
|
||
type CommitDict = Map<string, CommitNode>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분은 나중에 serialize 문제때문이라도 한번 생각해볼 필요가 있겠네요.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map#objects_vs._maps
return new Map( | ||
commits.map( | ||
(commit) => [commit.id, { traversed: false, commit } as CommitNode], | ||
[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 신기해서 찾아봤는데 Map 의 constructor 에서 iterable 을 지원하네요!
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map/Map
덕분에 하나 배워갑니다ㅎㅎㅎ
|
||
export function getLeafNodes(commitDict: CommitDict): CommitNode[] { | ||
const leafs: CommitNode[] = []; | ||
commitDict.forEach((node) => node.commit.branches.length && leafs.push(node)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 이부분은 forEach + inner filter 보다, filter + forEach 가 더 읽기 좋은것 같습니다 ㅎㅎ
|
||
push(node: T): void { | ||
this.queue.push(node); | ||
this.queue = this.queue.sort(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
확실히 concat 이랑 같이 보면 헷갈릴수는 있겠네요.
- concat 보다가 sort 볼때..!?
- sort 보다가 concat 볼때..??
혹시 이렇게 써버리고, this.queue는 unsorted 된 채로 인식할 수 있게 되기에... 저는 그냥 return type없이 쓰는걸 선호합니다 ㅎㅎ
이 의견에도 일부 동의합니다 ㅎㅎ 그냥 아무생각없이 코드만 딱 봤을때 원본자료는 보존되는것처럼 보일수있어서 자칫 실수가 일어날수도 있어보이긴합니다.
p.s.
사실 저도 sort Return Type 이 void 인줄 알았는데요. 확인해보니깐 또 아니네요.. ㅠㅠ
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/sort#return_value
import { CommitNode } from "./types/CommitNode"; | ||
import { CommitRaw } from "./types/CommitRaw"; | ||
|
||
const dummy = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분의 type 도 CommitRaw[] 로 지정하면 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anpaul0615 만약에 타입을 지정해 준다면 Pick<CommitRaw, "id" | "parents" | "branches">[]
로 해주고 싶어요.
요 데이터에 CommitRaw[]를 지정해 주면 필드를 덜 채워서 타입 에러가 납니다 🥲
어차피 다른 필드는 STEM 생성에서는 의미 없는 부분이라, fakeData 값에는 테스트를 위해 꼭 필요한 값만 채우고 아래의 createTestCommit
함수로 나머지 값을 넣어 CommitRaw를 완성시키려고 했어요.
packages/analysis-engine/src/stem.ts
Outdated
tailId: string, | ||
commitDict: Map<string, CommitNode>, | ||
q: Queue<CommitNode>, | ||
implicitBranchCount: number |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아마 N개의 implicitBranch 에 대해서 넘버링 할당하신 것 같아요.
말씀하신대로 이름이 count 가 적절하지 않을수도 있겠네요ㅎㅎ
packages/analysis-engine/src/stem.ts
Outdated
q: Queue<CommitNode>, | ||
implicitBranchCount: number | ||
): CommitNode[] { | ||
let now: CommitNode | undefined = commitDict.get(tailId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guard Clauses 참고자료 덧붙입니다! (알고계실수도있지만ㅎㅎ)
https://refactoring.com/catalog/replaceNestedConditionalWithGuardClauses.html
export interface Stem { | ||
id: string; | ||
headId: string; | ||
tailId: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이건 어떤 의미일까요??
아마 StemDictionary 를 branch 명으로 접근하는 부분일 것 같습니다.
stemDict = { [branch] : stem }
구조에서
이미 branch 를 알고있는 상황에서 stem 을 찾아온건데
stem 안에 branch(=stem.id) 가 있을 필요가 없다 라는 의미로 보입니다.
export function generateCommitNodeDict(commits: CommitRaw[]): CommitDict { | ||
return new Map( | ||
commits.map( | ||
(commit) => [commit.id, { traversed: false, commit } as CommitNode], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(질문) traversed: false
는 어떤 속성인가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hy57in STEM을 만들 때, 브랜치 포인터가 가리키는 leaf 노드부터 순회하기 시작하는데,
순회를 했는지 여부를 표시하기 위해 필요한 속성입니다.
PR에 함께 첨부하신 그림을 보시면, 처음 큐에 초록색으로 표시된 6, 9', 16번 노드가 enqueue 되는데요,
6번부터 순회하기 시작하면 [1,2,3,4,5,6] 이라는 STEM이 생성돼요.
그 다음으로 9'번부터 순회했을 때 [11,12,13,14,9'] 이라는 STEM이 생성되어야 하는데,
만약 traversed 체크를 하지 않는다면 3번을 순회했는지 여부를 (다른 STEM을 확인하지 않고서는) 알 수 없기 때문에 체크가 필요해요.
그런데 쓰고보니 traversed 속성 대신에 CommitNode.stemId 속성 null 체크하면 되겠네요 🤔
직접 만드신 그림 너무 대단하셔요~! 덕분에 이해하는 데 엄청 도움될 것 같습니다 감사합니다!!👍👍👍 |
정말 고생하셨습니당 |
닫고 새 PR로 갑니다 🚀 리뷰 감사합니다~! |
TODO
함께 고민해주신 @anpaul0615 님 감사합니다 👍