user 테이블 변경, studyLog 테이블 추가 #9

Manny9554 · 2021-07-26T12:50:50Z

바뀐 사항

ORM으로 갈아엎었음
DB 스키마 갈아엎었음

예정 사항

DB 결과를 결과값을 받도록 변경. (Enum형)
bot의 메세지도 결과값을 바탕으로 생성.

서브커맨드?는 더 찾아보겠음

예정 사항은 수요일 작업할 예정.

poetry

WraithKim

파일명에 대한 네이밍 컨벤션이 없긴 한데 내가 아는건 스네이크 케이스가 맞음. 그리고 컬렉션 역할의 폴더명은 복수형.
table/studyLog.py => tables/study_log.py

~~bot.py를 리팩토링해서 main.py 만든거 같은데 그럼 걍 bot.py 없애셈. 어차피 git에 남아서 다시 볼 수 있음.~~
예전부터 두 개였구나;;; 그래도 여기서 싹을 뽑아야 겠다. 없애주셈... ㅈㅅ

hi, bye, mylog 명령어는 테이블에 들어가는지는 정상동작 케이스만 직접 SQLite로 연결해서 테스트해봄. 돌아가는건 확인함. DB 함수에 대한 테스트랑 예외 케이스는 아직 안 해봄.

여기까지가 내 집중력의 한계다...

WraithKim · 2021-07-27T06:15:43Z

table/user.py

+class User(Base):
+    __tablename__ = "user"
+
+    name = Column(String(20), primary_key=True)


Discord 사용자의 이름은 수정이 가능함. 따라서 기본키로 사용해선 안됨.

사용자 이름 대신 사용자 ID를 넣는 것. discord.py에 User.id 필드가 있음.

(선택사항) 사용자 ID를 기본키로 두지 말고, 그냥 auto increment 하는 int 값을 둘 것.

2번의 이유

id라는 기본키는 정말로 거의 바뀔 일이 없음. 따라서 이 id를 가리키는 외래키도 바뀔 필요가 없음. ex) StudyLog의 User를 가리키는 외래키

만약에 Discord의 사용자 구분방식이 바뀐다면 기본키 컬럼을 갈아엎어야 된다는 불상사를 막기 위함

아 id 필드가 있으면 그걸로 하면 되겠네. 사실 이름 변경 기능 때문에 이름 필드말고 뭔가 다른 구분자가 있었으면 싶었는데, 몰라서 일단은 냅둔거였음.

WraithKim · 2021-07-27T06:31:21Z

main.py

+tb_name = os.getenv("DISCORD_DB_TB")
+admin_name = os.getenv("DISCORD_ADMIN")


이건 이전 코드의 잔재니까 지워주면 고맙...

WraithKim · 2021-07-27T06:33:24Z

main.py

+
+bot.run(token)


스크립트가 실행될 때의 코드라면 if __name__ == "__main__":
예전 코드랑 같긴 한데 그냥 Diff에 떠서 신경쓰였음.

요건 이번 커밋에서 빠졌는데 다음 PR때추가하는걸로?

main.py

WraithKim · 2021-07-27T06:47:22Z

utils/handle_db.py

return? 사용자한테 메세지를 보내고 싶었던 거 같은데... 이건 나도 고민좀 해볼게.

이거 내가 예정사항에 리턴값 줄거라고 했는데 일단 구상만 해놓은 문장임.

main 에서 end study method 에 대해서 기대하는 return value 가 없으므로 return 때리긴 조금.. 차라리 boolean을 주거나 -1을 주고 main에서 -1 리턴 시 예외 처리 이런 방식이 나아 보이는 데?

@comnamu18
그런 식으로 해놓을 예정. 예외처리하면서 주석으로 쓸 부분을 저렇게 써놨음.

더 손이 가지만 좋다고 생각하는 방법은 Exception으로 뱉는거...
Exception 그냥 상속만 한 NotFoundException 클래스 만들어서 raise 하는것도 좋음.

최종적으론 동의, 현재로서는 일단 LGTM

main.py

WraithKim

아래 내용은 의견 제시임.
DB의 성공여부를 따지는 부분부터 사용자 메세지까지의 코드가 복잡해졌다고 생각함.

성공 여부를 반환할 때의 복잡함

DB 접근 함수들이 성공여부를 반환값으로 보내는 건 적절하지 않다고 봄. 특히 query 함수는 ResultSet을 반환해줘야 하는데 그러면 실패할 경우를 알려줄 수 없음.

물론 지금 query함수는 Exception을 던지긴 함. 근데 그러면 왜 다른 함수들은 Exception이 아닌 반환값으로 알려줄까? 코드를 보는 사람 입장에선 일관성이 떨어진다고 생각함.

지금 당장 구현된 함수들은 성공하면 그냥 조용히 return하고 아닌 경우에만 알려줘도 되는 단순한 방법도 충분하다고 봄.

그래서 반환형에 구애받지 않고 실패한 경우에만 신경쓰는 예외처리가 더 간단한 방법이라고 생각함.

예외 처리 위치도 호출받은 쪽보단 호출한 쪽까지 보내서 처리하는 게 좋다고 봄. 호출한 쪽이 문제를 알아야 하기 때문. (main.py의 함수가 처리해야 한다는 의견)

사용자에게 보낼 메세지와 Enum 형

메세지도 Enum으로 정의되어 있는데 아직 메세지가 재활용되는 경우는 없음.

이 말은 만약 앞으로도 각각의 메세지가 한번만 쓰인다면, Enum으로 정의하는게 그냥 쓰는거랑 다를게 없는데 더 복잡한 방법이 된다는 거임.

따라서 사용자 메세지를 여러번 재활용해야 하는 경우가 생기지 않는다면 아직까진 그냥 명령어 함수에 적힌 문자열인 걸로도 충분하다고 봄.

그리고 db_msg.END_STUDY_FAILED 오타남.

WraithKim · 2021-07-30T15:58:24Z

tables/user.py

@@ -0,0 +1,26 @@
+from sqlalchemy.ext.declarative import declarative_base
+from sqlalchemy.orm import relationship


문제점: relationship을 import 했는데 사용하지 않음. 그리고 StudyLog와 User는 관계가 있는데 쓰지 않음.
잠깐 SQLAlchemy 문서 읽어보니까 relationship은 객체 그래프 탐색을 할 수 있게 해주는 거라고 되어있음.
특히 cascade 설정을 할 때 필요함.
(study_log에도 import 되어 있는데 쓰지 않음.)

foreign key 적용하려던 잔재 같은데 삭제하겠음.

WraithKim · 2021-07-30T16:02:15Z

tables/study_log.py

+
+    id = Column(Integer, autoincrement=True, primary_key=True)
+    user_id = Column(Integer, ForeignKey(User.id))
+    user_name = Column(String(20), ForeignKey(User.name))


StudyLog에는 user_name이 없어도 된다고 생각함.

생각해보니 그렇네

WraithKim · 2021-07-30T16:39:42Z

utils/handle_db.py

+    def end_study(self, user, current_time):
+        user_id = user.id
+        user_name = user.name


사용하지 않는 변수

놓쳤습니다. 삭제하겠습니다.

comnamu18 · 2021-07-31T17:51:25Z

forked repo 라 black 돌고 나서 commit & push 할때 권한 문제가 생기네..
https://stackoverflow.com/questions/58221321/is-github-actions-available-on-forked-repositories

일단 이건 받고
현 레포에서 branch따서 작업하는 게 나을 듯? 공유되는 코드를 개발하는 건데 굳이 fork를 뜰 필요는 없을 듯

관련 이슈 링크 보고 대충 따라서 넣어서 메인에 넣어봤는데 바로 에러나네..나중에 다시 생각해보겠습니다.

Manny9554 · 2021-07-31T18:00:36Z

@WraithKim

handle_db 메소드들의 일관성이 없어짐

이건 나도 코드를 작성하고 나서 느꼈음. 그런데 나는 main.py를 로직이나 데이터 후처리 과정은 배제하고 최대한 단순하게 만드는 게 맞다고 생각했음. 그러니 일단 아래의 생각들은 전부 'main.py에서는 최대한 로직을 배제해야 한다'는 생각을 전제로 한 것이라는 걸 염두해서 읽어줬으면 좋겠음.

main.py는 호출만 하고, 리턴 받은 값을 단순하게 출력(ctx.send)만 하게 하는 것이 좋다고 생각한 것이 지금의 !hi나 !bye의 모습. alluser나 mylog 같은건 디버그용 함수의 성격이 강해서 아직 안 건드리고 있었음.

사실 스프링처럼 컨트롤러(main) - 서비스(새로 작성) - 레포지토리/DAO(handle_db) 구조로 나누는 것도 생각해봤는데, 이 정도까지 구조를 나눌 필요가 있나 싶어서 일단 보류했음.

일단 내가 생각한 다른 방법은 메소드명으로 리턴값의 형식을 표현하는 것임.
지금 당장 봐도 row의 resultset을 기대하는 메소드들은 get으로 시작하고 있음 (get_all_users, get_my_study_logs)
그렇다면 일단 크게 resultset(내지는 그 resultset을 가공해서 만든 문자열)을 리턴할 메소드 / Enum을 리턴할 메소드 두 형식으로 나눌 수 있는데, 이 메소드들의 접두사 또는 접미사를 통일하는 형식으로 표현할 수 있지 않을까?

메세지랑 enum을 묶은 건 추후 확장이랑 관리를 용이하게 하려고 한 것.

위랑 이어지는 건데, main.py의 동작을 최대한 단순하게 하고 싶었음.
새로운 메소드를 작성하게 된다면 새로운 Enum과 (필요하다면) 그에 대응하는 메세지를 main.py와는 분리된 파일에 작성하고,
main.py에서는 메세지를 불러올 때 일관된 방법인 db_msgs.get(new_method())의 형태로 불러오면 되므로 이것이 더 용이할 것이라고 생각해서 지금과 같은 방법으로 분리하였음.

WraithKim · 2021-08-01T18:07:59Z

말이 점점 길어지고 나도 문장력의 한계가 와서 말로 하는게 전달하기 쉬운 거 같음. 디코로 코드 보면서 얘기하는게 더 빨리 끝날 거 같은데 그렇게 하쉴?

main.py가 컨트롤러이고 결국 퍼사드 패턴마냥 실제 로직은 다른 곳에서 돌아가고 main.py에서는 호출만 해주는 식이란건 동의함. 이 구조를 지키는 정도만 해도 난 단순하게 짰다고 봄. 여기서 더 단순하게 줄여보려다가 오히려 복잡해지지 않기를 바라는 거.

일단 내가 생각한 다른 방법은 메소드명으로 리턴값의 형식을 표현하는 것임.
지금 당장 봐도 row의 resultset을 기대하는 메소드들은 get으로 시작하고 있음 (get_all_users, get_my_study_logs)
그렇다면 일단 크게 resultset(내지는 그 resultset을 가공해서 만든 문자열)을 리턴할 메소드 / Enum을 리턴할 메소드 두 형식으로 나눌 수 있는데, 이 메소드들의 접두사 또는 접미사를 통일하는 형식으로 표현할 수 있지 않을까?

이거는 내가 물어본 성공여부를 반환값으로 넘기느냐 실패한 경우만 예외로 넘기느냐가 일관적이지 않다는 문제와 관련이 없어보이는데 추가로 설명해줄 수 있음?

예외 처리를 주장하는 이유 하나 더 덧붙일게.
실패에 대한 걸 반환값으로 전달하는 거보다 예외 처리를 더 선호하는 또 하나의 이유는 콜 스택을 타고 전파시키기 쉽다는 거임. 예로 들어서 repository에서 오류가 발생했는데 예외 처리를 controller에서 해줘야 되는 경우엔 service에선 걍 예외를 무시하면 됨. 반환값이면 service에서도 실패값을 전달하는 코드를 구현해야 하고. 이런 이유도 고려해줬으면 함.

메세지랑 Enum을 묶는게 일관된 방법을 제공하려는 건 알겠음. 하지만 그 이유가 메세지를 불러오는 과정을 쉽게 하기 위해서인데, 그 전에 메세지를 불러오는 방법을 개선해야 되는 요구사항 또는 문제가 발생했는지 먼저 따져봐야 한다고 생각함.

메세지를 적는 가장 간단한 방법은 그냥 직접 때려박는거임. 근데 이게 비효율적이게 되는 건 같은 메세지를 여러 곳에 박아야 되는 경우가 생길 때임. 이럴 때, 같은 메세지를 출력하는 부분을 하나의 함수로 묶든지 해서 일관된 방법을 제공하려고 하지.

결국 여러번 썼을 때, 어느 쪽이 코드량이 줄어드냐의 얘기가 됨. 네가 제안한 방법은 여러 번 같은 메세지를 적을 때 효과적인 방법이지만, 한 번밖에 쓰지 않을 때는 단순히 때려박는 방법보단 많이 적어야 됨. 부차적인 단점으로 메세지를 한 곳에 묶었지만 처음 개발하는 입장에서 보면 main.py에서 저 메세지까지 1 depth 더 들어가야 구조를 알 수 있다는 복잡함도 생김.

발생할 수 있는 요구사항을 고려하는 것도 좋지만 오히려 간결한 코드가 가장 확장하기 좋고 관리하기 쉽다고도 생각함. 그래서 지금은 메세지를 그냥 쓰는 단순한 구조로 가다가 네가 제안한 방법이 정말로 필요할 때 쯤에 적용해도 늦지 않다고 보는 거임.

Manny9554 · 2021-08-02T07:11:34Z

대화를 통해서 합의한 결론

이번 PR은 close
커맨드 파트를 분리하면서 handle_db.py에 있는 try - except 구문을 커맨드 명령어 정의 함수로 빼내기
handle_db.py에서 Enum을 리턴하던 부분은 문제 발생시 exception을 하는 것으로 바꿈 -> 이에 따른 exception의 예외처리는 커맨드 명령어 정의부분에서 하게 됨.
Enum과 연동된 메세지는 일단 삭제하고 커맨드 명령어 정의 함수에 그냥 문자열
로깅 적용
github action 때문에 포크한 레포지토리에서 작업하지 않고 이 레포에서 브랜치 따서 작업하기로 함

Manny9554 added 21 commits July 8, 2021 13:25

문자열 겹따옴표로 통일

d7631d6

SQLAlchemy 의존성 추가

6238517

User 테이블 객체 작성

79f099c

datetime 객체를 활용하도록 시간계산 함수 작성

6227339

SQLAlchemy를 활용해 DB 작업 구현

3565695

orm활용 명령어 추가

fefe5f9

pymysql 의존성 추가

b3c1d29

mysql에 대응하도록 varchar값 지정, 객체 생성시 기본값 할당

ec2b9a6

sql 바꿀수 있도록 변경, 새로운 유저 대응 추가

b8f7690

orm 적용된 명령어들 다른 파일로 분리

61b08e7

db echo 제거

a7ab697

분리한 명령어들을 활성화하는 명렁어를 추가

bae9169

requirements.txt 삭제

667633b

Merge pull request #1 from seolking/main

6b56198

poetry

orm 적용을 위해 파일명 변경

4cce170

_orm에 있던 내용 옮겨옴. session 그때그때 생성하도록 변경. 로그 남기도록 변경

b1a83d6

파일명 변경

ba11d75

로그 테이블 추가

e7f1fd4

쓰지 않을 파일 삭제

a4fa036

바뀐 db_handler 적용 및 명령어 변경

13f40b3

!bye에서 user가 none인 경우 검사 추가

a1a7840

WraithKim requested changes Jul 27, 2021

View reviewed changes

WraithKim reviewed Jul 27, 2021

View reviewed changes

main.py Show resolved Hide resolved

Manny9554 added 7 commits July 30, 2021 20:13

패키지 인식 하위호환을 위한 __init__추가

070dda8

파일 네이밍 변경

9b671d1

안 쓰게 된 bot.py 삭제

d849855

폴더명 변경

5cfe964

__init__ 추가

4515bb2

discord.py의 user.id를 사용하도록 테이블 변경

32ba5ff

DB 결과값 Enum 추가

9a4eef1

Manny9554 added 3 commits July 30, 2021 21:13

DB Enum에 따른 메세지 딕셔너리 작성

14885a0

DB Enum을 리턴하도록 변경. SQLAlchemyError 예외처리 적용. user.id 사용하도록 변경

99c182a

user.id를 사용하도록 변경. Enum 결과값으로 만들어진 msg를 리턴하도록 변경

30d70df

WraithKim requested changes Jul 30, 2021

View reviewed changes

comnamu18 mentioned this pull request Jul 30, 2021

Update Github Action #10

Merged

피드백 받은 미사용/불필요한 코드 삭제

53c5a33

Manny9554 closed this Aug 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

user 테이블 변경, studyLog 테이블 추가 #9

user 테이블 변경, studyLog 테이블 추가 #9

Manny9554 commented Jul 26, 2021

WraithKim left a comment •

edited

Loading

WraithKim Jul 27, 2021

Manny9554 Jul 27, 2021

WraithKim Jul 27, 2021

WraithKim Jul 27, 2021

comnamu18 Jul 30, 2021

WraithKim Jul 27, 2021

Manny9554 Jul 27, 2021

comnamu18 Jul 27, 2021

Manny9554 Jul 28, 2021

WraithKim Jul 28, 2021 •

edited

Loading

comnamu18 Jul 28, 2021

WraithKim left a comment •

edited

Loading

WraithKim Jul 30, 2021

Manny9554 Jul 31, 2021

WraithKim Jul 30, 2021

Manny9554 Jul 31, 2021

WraithKim Jul 30, 2021

Manny9554 Jul 31, 2021

comnamu18 commented Jul 31, 2021 •

edited

Loading

Manny9554 commented Jul 31, 2021 •

edited

Loading

WraithKim commented Aug 1, 2021 •

edited

Loading

Manny9554 commented Aug 2, 2021 •

edited

Loading

		tb_name = os.getenv("DISCORD_DB_TB")
		admin_name = os.getenv("DISCORD_ADMIN")

		@@ -0,0 +1,26 @@
		from sqlalchemy.ext.declarative import declarative_base
		from sqlalchemy.orm import relationship


		bot.run(token)

user 테이블 변경, studyLog 테이블 추가 #9

user 테이블 변경, studyLog 테이블 추가 #9

Conversation

Manny9554 commented Jul 26, 2021

WraithKim left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WraithKim Jul 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WraithKim left a comment • edited Loading

Choose a reason for hiding this comment

성공 여부를 반환할 때의 복잡함

사용자에게 보낼 메세지와 Enum 형

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

comnamu18 commented Jul 31, 2021 • edited Loading

Manny9554 commented Jul 31, 2021 • edited Loading

handle_db 메소드들의 일관성이 없어짐

메세지랑 enum을 묶은 건 추후 확장이랑 관리를 용이하게 하려고 한 것.

WraithKim commented Aug 1, 2021 • edited Loading

Manny9554 commented Aug 2, 2021 • edited Loading

WraithKim left a comment •

edited

Loading

WraithKim Jul 28, 2021 •

edited

Loading

WraithKim left a comment •

edited

Loading

comnamu18 commented Jul 31, 2021 •

edited

Loading

Manny9554 commented Jul 31, 2021 •

edited

Loading

WraithKim commented Aug 1, 2021 •

edited

Loading

Manny9554 commented Aug 2, 2021 •

edited

Loading