-
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: implement backend client #1267
Conversation
feat(talk-client): initialize database on first connection
feat(talk-client): adjust error message
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.
-
RowIndex를 &str 대신 숫자를 쓰는 이유는 뭔가요? 가능하면 가독성이 높은 &str 쪽으로 통일하면 좋겠습니다.
-
DB 스키마를 문서화할 필요가 있습니다. ORM을 사용하지 않으므로 코드에서 DB 스키마를 추측하려면 꽤나 많은 양을 읽어야 합니다. (추가 PR 권장) : 후속 이슈 DB 스키마 문서화 #1484
-
crate 간의 구조, 서버 - 클라이언트 소통 구조 등의 아키텍처적인 부분을 문서화할 필요가 있습니다. (추가 PR 권장) : 후속 이슈 아키텍처 문서화 #1485
-
문자열에 SQL을 쓰는 결정을 내렸으므로 sql 변경 시 validity 검증을 위한 리뷰 프로세스나 도구 등을 구비해야 합니다. (팀 내 문화 설정, 추가 PR 가능성 있음) : 후속 이슈 SQL 검증 프로세스 확립 #1486
-
The error message given by the Display representation of an error type should be lowercase without trailing punctuation, and typically concise.
https://rust-lang.github.io/api-guidelines/interoperability.html?highlight=message#error-types-are-meaningful-and-well-behaved-c-good-err
위 규칙에 따라, 에러 메시지는 소문자로 시작하는 편이 좋습니다.
에러 메세지의 경우 af5aab8 에서 전부 소문자로 시작하도록 고쳤습니다. RowIndex의 경우 from_row가 구조체와 정확히 한 형식의 쿼리에만 대응되고 숫자를 사용하는게 더 빠르기 때문에 &str를 사용하는 의미가 크게 없는것 같아 바꾼건데 다시 &str을 사용하는게 좋을까요? |
DB 스키마가 잘 문서화된다면 어느쪽을 택하든 괜찮습니다만 스키마가 문서로 없는 상황에선 컬럼 이름과 인덱스 사이 대응 관계가 코드만으론 바로 파악되지 않다보니 제안드린 사항이었어요. 저희가 prepared statement 등에서 WHERE로 필터링하거나, select로 선택할 떄에는 이름을 사용하게 될 가능성이 높고, 숫자와 이름이 번잡하게 공존하는 것보다는 이름만으로 통일하는 편이 좋을 것 같아요. |
앞서 단 리뷰 코멘트의 내용을 이슈로 만들었습니다. |
키위톡 로코 클라이언트의 기본 기능을 구현합니다.
변경사항
이 PR은 #1254 가 완료되기 전까지 merge 할 수 없습니다.