Skip to content

refact: lang2sql CLI 관련 코드 리펙토링 #70

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 1 commit into from
Apr 28, 2025

Conversation

ParkGyeongTae
Copy link
Contributor

#️⃣ Issue Number

📝 요약(Summary)

  • URL 설정 및 Streamlit 실행을 명확히 분리
  • 에러 핸들링 추가
  • 타입 힌트 추가
  • docstring 추가
  • click 기반 확장성 유지
  • 코드 스타일 통일

💬 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.

👍 정상실행 확인했습니다. 기여해주신 코드가 정말 깔끔하고 한눈에 들어옵니다. 많이 배우겠습니다.

image

@ParkGyeongTae
Copy link
Contributor Author

👍 정상실행 확인했습니다. 기여해주신 코드가 정말 깔끔하고 한눈에 들어옵니다. 많이 배우겠습니다.

image

@ehddnr301
저도 이 프로젝트로 많이 배우고 있습니당!
click 라이브러리는 처음보는데 이런 라이브러리도 있어서 신기하더라구여!
더 깔끔한 방법이 있는지 고민해보겠습니당~

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.

👍함수마다 주석 설정 필요하다고 생각했는데~ 감사합니다!

@click.option("-p", "--port", type=int, default=8501, help="Streamlit port")
def cli(ctx, datahub_server, run_streamlit, port):
# pylint: disable=redefined-outer-name
def cli(
Copy link
Contributor

Choose a reason for hiding this comment

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

👍: cli함수로 설정 변경한 거 좋은 것 같습니다

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nonegom 감사합니당~~!

@ParkGyeongTae
Copy link
Contributor Author

@nonegom 확인했습니다 감사합니당~~!

@ParkGyeongTae ParkGyeongTae merged commit a59caa3 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