-
Notifications
You must be signed in to change notification settings - Fork 0
Add issueStat query and openIssueCount field
#175
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #175 +/- ##
==========================================
+ Coverage 30.55% 35.01% +4.45%
==========================================
Files 10 11 +1
Lines 360 397 +37
==========================================
+ Hits 110 139 +29
- Misses 250 258 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
877023a to
bad7e98
Compare
|
@kimhanbeom 요거 저 대신 리뷰 진행해주실 수 있을까요? |
| login | ||
| } | ||
| } | ||
| assignees(first: 10) { |
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.
#173 (comment) 에서와 동일한 쟁점이 여기에도 있는 것 같습니다.
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.
특정 갯수로 제한하지 않고, 전부 다 가져올 수 있는 generic하면서 programatic한 방식을 도입하고, 이를 여러개의 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.
페이지네이션 전략이 다양할 수 있어서 고민이 좀 필요할 것 같네요. 이슈를 따로 생성하고 한꺼번에 적용하는 게 좋을 것 같습니다. 당장 생각나는 방법은
- 요청한 개수와 응답 받은 개수 비교해서 추가 요청하기
- 초과할 가능성이 높은 필드만 추가로 확인해서 요청하기
등이 생각나네요.
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.
요청한 개수와 응답 받은 개수 비교해서 추가 요청하기 방식을 취하면, 초과할 가능성이 높은 필드만 추가로 확인해서 요청하기 방식은 무조건 커버가 될 것으로 보입니다. 전자가 더 나은 방식이라고 생각합니다. 이슈를 생성해주실 수 있을까요?
한편, pagination이 결부되는 GraphQL field들은 이 PR에서 제거하고 리뷰&머지 하거나, 아니면 이 PR 자체를 pending 해두어야 할 것 같습니다. 아니면 이 부분을 TODO 로 주석에 표기해두고, 기술부채가 남지않도록 이를 챙기면 될 것 같습니다. 이 PR 진행방식은 저는 어떤 것이든 무관하다고 생각합니다. Jake께서 github-dashboard에서의 주요 2개 프로젝트 고려해서 정하면 될 것 같다고 생각합니다.
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.
TODO 주석 추가했습니다.
bad7e98 to
d0cc108
Compare
src/graphql/issue_stat.rs
Outdated
| #[derive(SimpleObject)] | ||
| struct IssueStat { | ||
| /// The number of open issues. | ||
| open_issue_count: usize, |
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.
usize는 GraphQL 타입 Int 범위를 넘어설 것 같습니다. aicers/review-web#37 이슈를 참고해주실 수 있을까요?
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.
반영했습니다.
| github::{issues::IssueState, GitHubIssue}, | ||
| }; | ||
|
|
||
| scalar!(IssueState); |
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.
scalar! 를 활용 했을 때, IssueState가 github-dashboard 클라이언트에게 enum 방식으로 표현되나요 아니면 string으로 표현되나요? 만약 string 이라면 이것이 enum 방식으로 표현되는 방법이 있을까요?
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.
Custom scalar 로 표현됩니다.
enum으로 표현하려면 enum을 직접 구현하고 변환 코드를 넣어야 됩니다.
Remote enum 이라는 게 있긴한데, 아래의 이유로 적용 불가능합니다.
- 적용하면 에러 발생: GraphQL enums may only contain unit variants.
- 원인은
IssueState에 unit variants 가 아닌Other(String)이 있기 때문 graphql-client에Other(String)이 하드코딩 되어있어서 제거할 수 없음
#175 (comment) 이 내용 찾아보다가 다른 일 잠깐 하느라 놓쳤네요. 이 PR에 |
7248642 to
d9a1ad7
Compare
리베이스 했습니다. |
4c1671f to
46bc948
Compare
src/graphql/issue_stat.rs
Outdated
| /// Format: "yyyy-MM-ddTHH:mm:ssZ" | ||
| begin: Option<DateTimeUtc>, | ||
| /// End of the creation datetime range. (exclusive) | ||
| /// Format: "yyyy-MM-ddTHH:mm:ssZ" |
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.
Format: "yyyy-MM-ddTHH:mm:ssZ" 으로 표현하면 꼭 해당 포맷이어야 하는 것으로 오해될 수 있을 것 같습니다. Example format 이라고 하거나 아예 없는 것이 더 좋을 것 같습니다.
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.
Example format으로 변경했습니다.
46bc948 to
fa6ab43
Compare

Closes #145