fix: reference definition node #887
Conversation
| modified: true, | ||
| deleted: false, | ||
| title | ||
| }; | ||
| } |
There was a problem hiding this comment.
@dongwoo-kim @seonim-ryu
~~중복 참조 정의 등록 방지 부분은 여기서 중복되는 노드에 대해 소스 포지션을 이용하여 별도의 처리를 해주면 될 것 같습니다만, 이슈 공유드린 것처럼 불합리한 부분이 있고 메모리 관련 관리 포인트가 하나 더 생겨 우선은 개발을 진행하지 않았습니다. 흔하지 않은 경우일 것 같지만, 의견 주시면 감사하겠습니다~!~~
|
[4/7] 리뷰 완료합니다ㅎ 참조 정의 기능이 생각했던 것 이상으로 어렵군요 ;ㅅ; 고생하셨어요! |
js87zz
force-pushed
the
fix/reference-definition
branch
from
39b2265 to
230b591
Compare
| if (quotationCount === 2) { | ||
| lastLineOffset = { line: i, ch: line.length }; | ||
| break; | ||
| } |
There was a problem hiding this comment.
참조 링크가 아래처럼 라인을 넘어 확장되어 사용될 수 있어 해당 부분을 고려한 로직들이 들어가 있습니다.
[foo]: test
't
e
s
t'
// or
[foo]:
test
'test'
There was a problem hiding this comment.
파싱하는 과정에서 현재 위치(this.pos)를 계속 갱신하고 있을 텐데,
sourcepos 계산을 위해서 별도로 다시 파싱하는 로직이 들어가는 게 좀 이상하네요.
위의 경우가 문제라면, parseLinkTitle 함수 내에서 this.lineIdx와 tihs.linePosOffset 값만 잘 보정해주면
this.sourcepos를 사용해서 실제 위치값을 계산할 수 있을 거 같습니다.
There was a problem hiding this comment.
그리고 이런 경우를 고려한 로직이 추가되었다면, 테스트 케이스도 같이 있어야 할 것 같은데,
toastmark.spec.ts 외에는 별도로 작성된 테스트가 없는 것 같네요.
sourcepos 계산은 실수하기 쉬운 부분이라,
참조 링크 용으로 테스트 파일을 별도로 분리해서 케이스별로 테스트를 작성하면 좋을 것 같습니다.
There was a problem hiding this comment.
this.pos 는 단순하게 문자열 위치만 계산해주는거라 유의미한 정보로 보기 어려울 것 같고..파싱 시점이 다른 인라인과 다르게 이 부분은 block 파싱중에 호출되는 부분이라 저 값들을 보정해서 사용하기 어렵지 않을까요?
libs/toastmark/src/toastmark.ts
Outdated
| this.updateRootNodeState(); | ||
| this.removeUnlinkedCandidate(); | ||
| this.replaceWithRefDefCandidate(); |
There was a problem hiding this comment.
수정되는 노드와 중복되는 참조 링크 정의 노드 판단을 하기 위해 수정된 이전 노드를 삭제한후, 신규 노드(파싱된 결과)에 참조 링크 정의 가 있는 경우 추가합니다. 이후 중복된 참조 정의 링크를 판단하기 위해 관리하고 있던 맵을 이용해 노드를 다시 등록해준 후 연관된 노드들을 다시 파싱합니다.
| } | ||
| } | ||
|
|
||
| return [startNode, endNode, startLine, endLine] as const; |
There was a problem hiding this comment.
아래와 같은 케이스를 고려하기 위해 추가되었습니다.
-
참조정의 링크 노드의 타이틀이 다음 행에 입력되는 경우
-
위의 노드와 함께 문자열로 합쳐지는 경우
| if (initRefMap) { | ||
| this.refMap = {}; | ||
| this.refLinkCandidteMap = {}; | ||
| this.refDefCandidteMap = {}; | ||
| } |
There was a problem hiding this comment.
parse('foo', true)로 호출된 다음 parse('foo', false)로 호출되는 경우는 없나요??
최초에만 1번 parse()가 호출되는 구조인지 조금 헷갈리는데.. 그게 아니라면 initRefMap = false 일 때 refMap, refLinkCandidteMap, refDefCandidteMap의 초기화 값을 고려해야될 것 같습니다.
There was a problem hiding this comment.
parser만 단독으로 사용하는 경우는 이 옵션은 크게 의미 없을 것 같고, toastmark를 사용하는 경우에는 최초 생성시 map들의 값 설정을 해주기 때문에 그런 경우는 없을 것 같습니다!
libs/toastmark/src/toastmark.ts
Outdated
| removedNodeRange: !extStartNode ? null : [extStartNode.id, extEndNode!.id] | ||
| } as EditResult; | ||
| const refLink = this.parseRefLink(); | ||
| result = refLink ? result.concat(refLink) : result; |
There was a problem hiding this comment.
result 변수가 refLink 보다 먼저 선언된 이유는 타입 때문인가요??
There was a problem hiding this comment.
editResult 값이 먼저 파싱된 후 참조 링크가 파싱되어야 올바르게 파싱됩니다. 그렇지 않으면 참조 정의 링크 자체가 수정된 경우 수정사항이 올바르게 반영되지 않는 경우가 있습니다.
There was a problem hiding this comment.
이렇게 선언되었을 때 parseRefLink()를 호출하면서 editResult 값이 변경된다는거죠?ㅎ
그렇다면.. 오케이요ㅎㅎ
const refLink = this.parseRefLink();
const result: EditResult[] = refLink ? result.concat(refLink) : [editResult];There was a problem hiding this comment.
@seonim-ryu
최초 editResult이 변경된다기 보단 만약 아래처럼 참조 정의 노드가 변경된 경우 변경사항을 [foo] 와 같은 노드들에 적용하려면 refMap을 먼저 갱신한후 순차적으로 진행되어야 해서 그렇습니다.ㅋㅋ
[foo]: abc => [foo]: abcd
| @@ -381,10 +388,14 @@ export class Parser { | |||
| } | |||
|
|
|||
| // The main parsing function. Returns a parsed document AST. | |||
| parse(input: string) { | |||
| parse(input: string, initRefMap = true) { | |||
There was a problem hiding this comment.
initRefMap이 false인 경우가 있나요?
제 생각에 parse 함수는 전체를 파싱하니까 refMap을 매번 초기화해도 될 것 같은데요.
There was a problem hiding this comment.
toastmark 생성자에서 초기 값에 대해 parse 함수를 호출 하는데 이 경우에 초기화를 하면 이후 부분 파싱을 할때 참조 링크 정보가 안 맞기 때문에 추가하였습니다.
|
|
||
| constructor(contents?: string, options?: Partial<Options>) { | ||
| this.refMap = {}; | ||
| this.refLinkCandidateMap = {}; | ||
| this.refDefCandidateMap = {}; |
There was a problem hiding this comment.
코드를 전반적으로 살펴봤는데, 맵을 세 개나 관리하는 건 비효율적인 느낌이네요.
refLinkCandidateMap, refDefCandidateMap 둘 다 결국 특정 레이블에 종속되는 노드 정보라서
refMap 내부에서 관리되는 게 더 효율적일 것 같아요.
예를 들어 refMap에서 unlinked된 객체를 지우고 나면, 연관된 candidate 정보는 자동으로 같이 지워져서
불필요하게 또 순회할 필요가 없을 것 같습니다.
그리고 현재 id 값으로 접근하는 로직이 없고 순회할 때만 사용하는 것 같은데, 그럼 굳이 맵 형태로 관리할 필요도 없을 것 같네요.
제 생각에는 refMap 내부에 defCandidates 배열과 linkCandidates 배열 두 개를 관리하면 될 것 같은데.. 어떤가요?
이 때 defCandidates를 정렬된 상태로 관리하면 첫 번째 요소가 unlinked 될 때만 refMap 정보를 갱신할 수도 있을 것 같습니다.
There was a problem hiding this comment.
refMap안에서 다 관리하게 되면 동일 label의 링크 정의 노드를 수정하였을 때 이전 정의 노드의refLinkCandidateMap, refDefCandidateMap 정보를 새로 수정된 노드에 옮기는 작업이 추가적으로 들어가게 됩니다. 그리고 cadidate 자체가 사라지는 경우에 참조 정의 노드 내부 배열에 직접 접근하여 갱신해줘야 하구요.
이 작업이 생기면서 복잡도가 더 증가하고 비효율적이라 생각하여 별도의 맵으로 관리하는 방향으로 선택하였습니다.
js87zz
force-pushed
the
fix/reference-definition
branch
from
78acf76 to
edcb473
Compare
libs/toastmark/src/toastmark.ts
Outdated
| private parseRefLink() { | ||
| const result: EditResult[] = []; | ||
|
|
||
| if (this.useReferenceDefinition && !isEmptyObj(this.refMap)) { |
There was a problem hiding this comment.
이렇게 모든 분기 처리를 내부에서 하는 것보다 호출하는 쪽에서만 처리하는 게 낫지 않을까요?
그게 전체 흐름을 살펴볼 때 더 이해하기 편할 것 같습니다.
parseRefLink()는 호출 하는 쪽에서도 처리를 하고 있어서 굳이 내부에서는 분기할 필요가 없을 것 같아서요.
There was a problem hiding this comment.
그게 더 효율적이긴 한대..그렇게 처리했다가..너무 더럽고 못생겨져서요.. 호출하는 쪽에서 처리하는 걸로 변경할까요?
There was a problem hiding this comment.
호출 순서가 보장되어야 하는 부분들이 있어서 아래처럼 못생겨 지는 경우가 생기더라구요
// 변경 전
this.markDeletedRefMap(extStartNode, extEndNode);
this.replaceRangeNodes(extStartNode, extEndNode, newNodes);
this.replaceWithNewRefDefState(newNodes);
// 변경 후
if (this.useReferenceDefinition) {
this.markDeletedRefMap(extStartNode, extEndNode);
}
this.replaceRangeNodes(extStartNode, extEndNode, newNodes);
if (this.useReferenceDefinition) {
this.replaceWithNewRefDefState(newNodes);
}
// 또는
if (this.useReferenceDefinition) {
this.markDeletedRefMap(extStartNode, extEndNode);
this.replaceRangeNodes(extStartNode, extEndNode, newNodes);
this.replaceWithNewRefDefState(newNodes);
} else {
this.replaceRangeNodes(extStartNode, extEndNode, newNodes);
}There was a problem hiding this comment.
쩝. 그렇군요. 그래도 전 변경 후가 나은 것 같아요. 큰 흐름을 읽을 때는 더 이해가 쉽지 않을까요.
아님 아래처럼 처리해도 될 것 같습니다. ㅎㅎ
if (this.useReferenceDefinition) {
this.markDeletedRefMap(extStartNode, extEndNode);
this.replaceRangeNodes(extStartNode, extEndNode, newNodes);
this.replaceWithNewRefDefState(newNodes);
} else {
this.replaceRangeNodes(extStartNode, extEndNode, newNodes);
}
Please check if the PR fulfills these requirements
fix #xxx[,#xxx], where "xxx" is the issue number)Description
Thank you for your contribution to TOAST UI product. 🎉 😘 ✨