Skip to content

refact: streamlit 멀티 페이징 모듈 리펙토링 및 검증함수 추가 #72

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

Conversation

ParkGyeongTae
Copy link
Contributor

#️⃣ Issue Number

📝 요약(Summary)

  • 최상위 멀티 페이지 정의하도록 수정
  • 정의된 딕셔너리 검증함수 추가

💬 To Reviewers (선택)

  • None

PR Checklist

  • lang2sql --datahub_server http://datahub_ip:datahub_port run-streamlit

reference) How to Code Review

  • 따봉(👍): 리뷰어가 리뷰이의 코드에서 칭찬의 의견을 남기고 싶을 때 사용합니다.
  • 느낌표(❗): 리뷰어가 리뷰이에게 필수적으로 코드 수정을 요청할 때 사용합니다.
  • 물음표 (❓): 리뷰어가 리뷰이에게 의견을 물어보고 싶을 때 사용합니다.
  • 알약 (💊): 리뷰어가 리뷰이의 코드에서 개선된 방법을 제안하지만 그것의 반영이 필수까지는 아닐 때 사용합니다.

@ParkGyeongTae ParkGyeongTae self-assigned this Apr 26, 2025
Copy link
Collaborator

@ehddnr301 ehddnr301 left a comment

Choose a reason for hiding this comment

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

👍 정상작동 확인했습니다! 잘 구조화된것같아 추후 페이지를 추가할때도 너무 좋을 것 같습니다!!

💊 validate 함수는 interface directory 하위에서 분리하는것이 더 깔끔한 코드가 될것 같은데 보통은 어떻게 관리하나요? (몰라서 질문드립니다ㅎㅎㅎ..)

image

@ParkGyeongTae
Copy link
Contributor Author

👍 정상작동 확인했습니다! 잘 구조화된것같아 추후 페이지를 추가할때도 너무 좋을 것 같습니다!!

💊 validate 함수는 interface directory 하위에서 분리하는것이 더 깔끔한 코드가 될것 같은데 보통은 어떻게 관리하나요? (몰라서 질문드립니다ㅎㅎㅎ..)

image

@ehddnr301
페이지 추가를 고려해서 구조를 변경했는데, 의도한바가 전달되서 너무 감동입니당!!
저도 validate 함수를 작성하고 보니 하위로 분리하는게 깔끔할 걸 똑같이 느꼈는데, 다음 스텝으로 진행하겠습니다~!

저도 잘은 모르지만, 일반적으로는 custom validate 클래스 하나 안에 여러개의 validate 함수를 만들어서 가져다 쓰는 구조로 사용하지 않을까 싶습니다. 다만, 프로젝트가 점점점점 커져서 validate 함수가 엄~청 많아지면 해당 클래스도 분리하는 작업이 고려될만해 보여요!

Copy link
Contributor

@nonegom nonegom left a comment

Choose a reason for hiding this comment

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

👍: 저도 처음에 멀티 페이징 알려드렸을 때, 이렇게까지 모듈화를 생각하지는 못헀는데 좋은 것 같습니다!

@ParkGyeongTae
Copy link
Contributor Author

@nonegom 오 다행입니당!!! 더 확장성있도록 고려해보겠습니다~~

@ParkGyeongTae ParkGyeongTae merged commit 2433c3f into master Apr 28, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants