Skip to content

Feature/add getBillList function #12

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

Merged
merged 13 commits into from
Aug 2, 2024
Merged

Conversation

StatPan
Copy link
Collaborator

@StatPan StatPan commented Jul 31, 2024

lawmaker api 스타일과 비슷하게

getBillList 기능 구현해봤습니다

@StatPan StatPan added the enhancement New feature or request label Jul 31, 2024
@StatPan StatPan added this to the v0.0.1 필수 API 매핑 milestone Jul 31, 2024
@StatPan StatPan requested a review from sunrabbit123 July 31, 2024 16:25
@sunrabbit123
Copy link
Collaborator

행동력 개쩌네요

Copy link
Collaborator

@sunrabbit123 sunrabbit123 left a comment

Choose a reason for hiding this comment

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

오오오

@sunrabbit123 sunrabbit123 changed the title Feature/add get bill fn Feature/add getBillList function Aug 1, 2024
@sunrabbit123
Copy link
Collaborator

related #8.

Copy link

codecov bot commented Aug 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Files Coverage Δ
src/bill/getBillList.ts 100.00% <100.00%> (ø)
src/constant/index.ts 100.00% <ø> (ø)
src/types/callOpenApi.ts 100.00% <ø> (ø)

@StatPan
Copy link
Collaborator Author

StatPan commented Aug 1, 2024

@sunrabbit123 수정했는데 다시 한번 확인해보시죵
어우 익숙치 않은 영어

@sunrabbit123 sunrabbit123 self-requested a review August 1, 2024 09:34
@sunrabbit123
Copy link
Collaborator

? 분신술인가

@StatPan
Copy link
Collaborator Author

StatPan commented Aug 1, 2024

이전에 계정 만들었던게 있는데 삭제했는데 살아있어요 이상하게.. 로그인 안되는데
이메일 설정을 바꿨어요

Copy link
Collaborator

@sunrabbit123 sunrabbit123 left a comment

Choose a reason for hiding this comment

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

추가로 plenarysessionReviewResult 얘 아직도 수정 안됐어영

* @description implementation of 법률안 심사 및 처리 API
*/
export const getBillList = async ({ page, take, ...rest }: Argument) => {
const {
Copy link
Collaborator

Choose a reason for hiding this comment

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

엇, 제가 작성했던거야 사용자들이 argument 이해하기 쉽게 변수명 transform해서 넣어놨던건데
이럴거면 굳이 구조분해할당이 필요없지않나 싶읍니다.

Argument 프로퍼티명을 수정해보는건 어떨까요

import { describe, it, expect } from 'vitest';
import { getBillList } from './index';

describe('bill index.ts', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

엇 이게 필요한가요

Copy link
Collaborator

Choose a reason for hiding this comment

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

export, import 관련해서 vitest 테스트 커버리지 이슈라면 #15 에서 다루고 있습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

테스트 과정에서 문제가 있어서 저걸로 풀기는 했는데, vitest에 정확한 이해도를 가지고 있진 않아요
요거 판단해주시믄 감사하겠습니닷

Copy link
Collaborator

Choose a reason for hiding this comment

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

삭제하고 함 돌려보죠
저도 이해도를 가지고 있진않은데 해결이 되고, 타 라이브러리들 참고했을 때 istanbul 사용하더라고요

@sunrabbit123
Copy link
Collaborator

sunrabbit123 commented Aug 2, 2024

그리고 작업 완료하시면 여기에 Re-request review 버튼 있어요
안해도 알잘딱으로 리뷰가 진행되겠지만, 저걸 누르면 명시적으로 나 작업 다했으니 코드리뷰해줘! 가 됩니당
image

@StatPan
Copy link
Collaborator Author

StatPan commented Aug 2, 2024

오우 꿀팁 감사합니다
매번 골뱅이로 부를 뻔

};

type Argument = {
BILL_ID?: string; // 의안ID, 예시: BILL_ID='PRC_Z2Z1Z0Z3X2L4M0H9A2V6K5R0V7P2H1'
Copy link
Collaborator

Choose a reason for hiding this comment

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

argument 이름 통일하는게 좋을 듯 싶은데

여기처럼 Open API에 있는 인자값을 따라가는게 맞을까요
아니면 sdk기에 따로 변수를 convert하는편이 좋을까요?

저는 개인적으로 후자가 맞다고 봅니다. sdk기에

일단 이건 정해서 싹 수정하면 되는 내용이라 이야기만 하면 될 것 같아요

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

후자에 동의합니다
그렇게 하려고 하더라도 요청할 때 키값은 맞춰야 하니 정보 매핑해서 관리는 해야하는거죠?

Copy link
Collaborator

Choose a reason for hiding this comment

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

네네 그래서 함수딴에서 구조분해할당으로 매핑하는 코드가 있는거고요

@sunrabbit123 sunrabbit123 assigned StatPan and unassigned sunrabbit123 Aug 2, 2024
@StatPan StatPan requested a review from sunrabbit123 August 2, 2024 13:34
@StatPan
Copy link
Collaborator Author

StatPan commented Aug 2, 2024

@sunrabbit123
Argument 프로퍼티 이름 변경
객체구조 분해할당 제거
한번 확인해보실래요

@sunrabbit123
Copy link
Collaborator

음 기존 코드들도 해당 형태로 변환하도록 하죠 그럼

@sunrabbit123 sunrabbit123 merged commit a49d7c8 into main Aug 2, 2024
2 checks passed
@sunrabbit123 sunrabbit123 deleted the feature/add-get-bill-fn branch August 2, 2024 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants