Skip to content

Comments

Type annotation overhaul#1442

Draft
demberto wants to merge 2 commits intoTomSchimansky:masterfrom
demberto:improve_type_hints
Draft

Type annotation overhaul#1442
demberto wants to merge 2 commits intoTomSchimansky:masterfrom
demberto:improve_type_hints

Conversation

@demberto
Copy link
Contributor

Highlights:

  • Removed redundant imports and sorted them.
  • Type hinted almost all the functions in the entire code base (even some examples).
  • Corrected many wrong and incomplete type hints (unspecialised collections, missing optionals, etc).
  • Upgraded them to use __future__.annotations (3.7+ feature, which customtkinter supports).

This is a big PR, it will need a few inputs and revisions from you and me.
I have left type hinting to you or used Any at ambiguous places.
⚠️ There are quite a few places where redundant code is present.
⚠️ The unit tests raise a lot of errors and they need to be updated.

There are some glaring code quality issues all over; along with the lack of docstrings at many places.
I highly suggest you to integrate ruff and black in your pre-commit hooks or atleast in your workflow.


# update width and height attributes
width, height, x, y = self._parse_geometry_string(geometry_string)
width, height, *_ = self._parse_geometry_string(geometry_string)

Choose a reason for hiding this comment

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

What's this *_? I've never seen that before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

x and y were unused in that context. Python uses the convention of naming unused variables as _. *_ does a tuple unpack operation and can be thought of as an *args parameter used in functions.

Copy link
Contributor Author

@demberto demberto Apr 19, 2023

Choose a reason for hiding this comment

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

Also, there's multiple places in the code that require attention. Redundant None checks, incorrect default values to name a few. It prevents a complete type hinting of the codebase.
I am unfamiliar with the actual logic and considering that tkinter itself is a very poor API, I leave it as is

Choose a reason for hiding this comment

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

I discovered this library a month ago and have been making simple fixes and formatting changes in my fork. I need to do a deeper dive to learn how it really works/is supposed to work.

@elly-hacen
Copy link

Can you add toast messages functionality in CTk? 🥹

@ghost
Copy link

ghost commented Jun 13, 2023

Perhaps a Pylint Github action...

@FedericoSpada
Copy link
Collaborator

Hi,
You used the "type1 | type2" convention that has been implemented in Python 3.10. Your code is not compatible with Python 3.7.

I've planned to add Type Hints myself in the near future. If you don't want to redo the work again, since I like your work on the Import clauses, you could create a new PR dedicated to that.

@bfedie518
Copy link

Hi, You used the "type1 | type2" convention that has been implemented in Python 3.10. Your code is not compatible with Python 3.7.

I've planned to add Type Hints myself in the near future. If you don't want to redo the work again, since I like your work on the Import clauses, you could create a new PR dedicated to that.

__future__.annotations adds support for the type1 | type2 convention to Python 3.7

@FedericoSpada
Copy link
Collaborator

Oh, I didn't know that.
Is it considered good practice instead of sticking to the previous Optional[Union[...]] convention?

@bfedie518
Copy link

Oh, I didn't know that. Is it considered good practice instead of sticking to the previous Optional[Union[...]] convention?

Yeah. The pipe is used instead of Union[], and | None is preferred over Optional[].

@FedericoSpada
Copy link
Collaborator

I meant: for Python 3.7 specifically, is it good practice to use from __future__ import annotations and the new pipe convention, or is it better to stick to the convention that was present when that Python version was released?
Since newer versions are retro-compatible and still allow the old convention, it may be good practice for projects that should work across multiple Python versions to stick to the old one.

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.

4 participants