Skip to content

Typehint most of main.py, with consequent fixes elsewhere #1300

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rolandwalker
Copy link
Contributor

@rolandwalker rolandwalker commented Aug 2, 2025

Description

  • typehint >80% of main.py
  • requiring additional hints and fixes in config.py and sqlexecute.py, packages/parseutils.py, and others
  • typehint unhinted portions of sqlexecute.py
  • remove @export decorator and import explicitly into special
  • recast some uninformative variable names
  • assert that a connection is available before using it
  • take more care with: ssl config, port, and ssh_port types/defaults before creating a connection
  • local_infile is a boolean, not a string
  • ensure get_password_from_file() returns a value
  • assert that a PromptSession is available before invoking methods on it
  • set a fallback col/row size in case a prompt session is not available
  • declare and type self.conn in SQLExecute much earlier
  • take more care to distinguish falsey values from Nones in SQLExecute connect()
  • improve defaults passed to pymysql.connect(), matching them to the pymysql docs
  • test that self.conn is not None rather than checking for a conn attribute, before invoking close()
  • remove Python 2.x-compat Unicode literals trick for click
  • better catch empty database names in change_db()
  • take more care for edge-case values of text in one_iteration()
  • don't return None for connection_id_to_kill
  • clarify overlapping e variables
  • don't use self.sqlexecute when sqlexecute is already available
  • is_dropping_database() takes a string, not a list
  • typehint unhinted function in packages/parseutils.py
  • use only chain() for format_output() output
  • test isinstance(…, Cursor) rather than hasattr(…, "description")
  • set start = time() before try block
  • update changelog

@amjith I tried to keep the cute @export decorator but it is too dynamic for a static checker to understand.

Checklist

  • I've added this contribution to the changelog.md.
  • I've added my name to the AUTHORS file (or it's already there).
  • I ran uv ruff check && uv ruff format to lint and format the code.

@rolandwalker rolandwalker self-assigned this Aug 2, 2025
@rolandwalker rolandwalker force-pushed the RW/typehint-main-py-pass-one branch 5 times, most recently from 25fba82 to 248da22 Compare August 2, 2025 15:40
@rolandwalker rolandwalker changed the title WIP typehint main.py Typehint main.py, with consequent fixes elsewhere Aug 2, 2025
@rolandwalker rolandwalker changed the title Typehint main.py, with consequent fixes elsewhere Typehint some of main.py, with consequent fixes elsewhere Aug 2, 2025
@rolandwalker rolandwalker force-pushed the RW/typehint-main-py-pass-one branch from 248da22 to 7f66c01 Compare August 2, 2025 15:45
@rolandwalker rolandwalker requested a review from amjith August 2, 2025 15:45
@rolandwalker rolandwalker force-pushed the RW/typehint-main-py-pass-one branch 5 times, most recently from 3eaafb7 to 0e37fb3 Compare August 2, 2025 20:41
@rolandwalker rolandwalker changed the title Typehint some of main.py, with consequent fixes elsewhere Typehint most of main.py, with consequent fixes elsewhere Aug 2, 2025
@rolandwalker rolandwalker force-pushed the RW/typehint-main-py-pass-one branch from 0e37fb3 to 5eb91dd Compare August 2, 2025 20:43
'write_once',
'write_pipe_once',
'write_tee',
]
Copy link
Member

Choose a reason for hiding this comment

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

Non-blocking question. Do you think we need to hoist all these functions to the top level?

BTW, I'm fine with removing the @export decorator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we do need to hoist. It could be nicer not to.

 * typehint >80% of main.py
 * requiring additional hints and fixes in config.py, sqlexecute.py,
   packages/parseutils.py, and others
 * typehint unhinted portions of sqlexecute.py
 * remove @export decorator and import explicitly into "special"
 * recast some uninformative variable names
 * assert that a connection is available before using it
 * take more care with: ssl config, port, and ssh_port types/defaults
   before creating a connection
 * local_infile is a boolean, not a string
 * ensure get_password_from_file() returns a value
 * assert that a PromptSession is available before invoking methods on
   it
 * set a fallback col/row size in case a prompt session is not available
 * declare and type self.conn in SQLExecute much earlier
 * take more care to distinguish falsey values from Nones in SQLExecute
   connect()
 * improve defaults passed to pymysql.connect(), matching them to the
   pymysql docs
 * test that self.conn is not None rather than checking for a "conn"
   attribute, before invoking close()
 * remove Python 2.x-compat Unicode literals trick for click
 * better catch empty database names in change_db()
 * take more care for edge-case values of text in one_iteration()
 * don't return None for connection_id_to_kill
 * clarify overlapping "e" variables
 * don't use self.sqlexecute when sqlexecute is already available
 * is_dropping_database() takes a string, not a list
 * typehint unhinted function in packages/parseutils.py
 * use only chain() for format_output() output
 * test isinstance(…, Cursor) rather than hasattr(…, "description")
 * set start = time() before try block
 * update changelog
@rolandwalker rolandwalker force-pushed the RW/typehint-main-py-pass-one branch from 5eb91dd to 644d682 Compare August 4, 2025 10:50
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.

2 participants