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 base repository and fix funding api for executive feedbacks #1419

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

pbc1017
Copy link
Contributor

@pbc1017 pbc1017 commented Feb 2, 2025

요약 *

It closes #issue_number

스크린샷

이후 Task *

  • 없음

@pbc1017 pbc1017 linked an issue Feb 2, 2025 that may be closed by this pull request
2 tasks
@@ -300,7 +300,7 @@ export const zFundingCommentResponse = zFundingComment.extend({
chargedExecutive: zExecutiveSummary,
});

export const zFundingCommentRequestCreate = zFundingComment.omit({
export const zFundingCommentRequest = zFundingComment.omit({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

이거 omit 두번하는 패턴 봤는데 프론트 입장에서 가독성이 너무너무 안 좋을 것 같음. 그래서 도대체 무슨 필드가 있는 건데 싶을 거 같음 @Gerbera3090 고민 좀 필요할듯

Comment on lines +17 to +19
interface ModelStatic<T extends MEntity, D> {
fromDbResult(result: D): T;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

이름 변경하는게 좋을거 같음. static fromDbResult를 포함하는 model 이라는 뜻

Copy link
Contributor

Choose a reason for hiding this comment

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

factory 면 factory 라고 작명 ㄱㄱ

Comment on lines +109 to +122
async patchTx(
tx: DrizzleTransaction,
oldbie: M,
consumer: (oldbie: M) => M,
): Promise<M> {
const param = consumer(oldbie);
await tx
.update(this.table)
.set(param)
.where(eq(this.table.id, oldbie.id))
.execute();

return this.fetchTx(tx, oldbie.id);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

patch의 방향이 Partial 대신에 consumer 로 갔으면 좋겠음. 이렇게 했을 때 메리트는 단순히 필드 하나 바꾸는 동작 이상의 뭔가를 할수 있음. consumer 안에서 조건문을 쓰거나, 모델의 클래스 메서드를 호출하거나 등등 활용성이 아주 좋아짐.

Copy link
Contributor

Choose a reason for hiding this comment

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

구상은 이해하는데 실용적으로 사용가능할지 더 사용 필요할듯

Copy link
Contributor Author

@pbc1017 pbc1017 Feb 2, 2025

Choose a reason for hiding this comment

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

funding.comment.type.ts와 funding.type.ts 간의 순환참조 문제를 해결하기 위해 별도 파일로 분리

Copy link
Contributor

Choose a reason for hiding this comment

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

common은 너무 가독성 떨어지는 거 같은데
순환 참조 문제는 나도 알고 있는데 다른 해결법 없나?

@pbc1017 pbc1017 self-assigned this Feb 2, 2025
@@ -13,7 +13,7 @@ import {

import { Activity, ActivityD } from "./activity.schema";
import { Club } from "./club.schema";
import { ExecutiveT, StudentT } from "./user.schema";
import { Executive, StudentT } from "./user.schema";
Copy link
Contributor

Choose a reason for hiding this comment

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

엥 이거 왜 바꼈죠

Comment on lines +109 to +122
async patchTx(
tx: DrizzleTransaction,
oldbie: M,
consumer: (oldbie: M) => M,
): Promise<M> {
const param = consumer(oldbie);
await tx
.update(this.table)
.set(param)
.where(eq(this.table.id, oldbie.id))
.execute();

return this.fetchTx(tx, oldbie.id);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

구상은 이해하는데 실용적으로 사용가능할지 더 사용 필요할듯

Comment on lines +17 to +19
interface ModelStatic<T extends MEntity, D> {
fromDbResult(result: D): T;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

factory 면 factory 라고 작명 ㄱㄱ


async find(id: number): Promise<M | null> {
return this.withTransaction(async tx => this.findTx(tx, id));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

오버로딩도 잘 처리되나?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

아주 잘되고, 예시 만들어둠

Copy link
Contributor

Choose a reason for hiding this comment

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

common은 너무 가독성 떨어지는 거 같은데
순환 참조 문제는 나도 알고 있는데 다른 해결법 없나?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fix funding api for executive feedbacks
2 participants