python-plexapi icon indicating copy to clipboard operation
python-plexapi copied to clipboard

add type hints to `base.py`

Open Dr-Blank opened this issue 2 years ago • 5 comments

Description

Support mypy and/or pyright for the base.py

related #1296

Type of change

Please delete options that are not relevant.

  • [x] New feature (non-breaking change which adds functionality)

Checklist:

  • [x] My code follows the style guidelines of this project
  • [x] I have performed a self-review of my own code
  • [x] I have commented my code, particularly in hard-to-understand areas

Dr-Blank avatar Nov 16 '23 09:11 Dr-Blank

Is there any task remaining for this PR?

Dr-Blank avatar Dec 31 '23 14:12 Dr-Blank

I also created a new branch for typehints. I think it would be better to finish type hinting the entire library before merging into master.

JonnyWong16 avatar Jan 16 '24 00:01 JonnyWong16

should I break this PR further down? Maybe on a per function basis? That should make it easier for review as currently this change is maybe so huge that it is daunting to start reviewing, and multiple threads going on for each change.

Dr-Blank avatar Mar 03 '24 19:03 Dr-Blank

I also created a new branch for typehints. I think it would be better to finish type hinting the entire library before merging into master.

I'm considering whether it would be more beneficial to merge it into master directly. Here are my thoughts:

  1. Even adding type hints to only base.py could greatly enhance readability, providing immediate benefits to users and contributors with making new PRs.
  2. By the time the entire project is type hinted, both branches may have diverged significantly, potentially leading to merge conflicts. Merging directly could mitigate this issue.
  3. I'm planning to break down PRs on a per-function basis. Since type hints are primarily for developers and don't affect code execution, they're unlikely to introduce breaking changes.

@JonnyWong16, I'd appreciate your thoughts on this. Do you think the benefits of direct merging outweigh any potential risks?

either way I will close this PR and start anew since this is too huge an undertaking for review.

Dr-Blank avatar Apr 16 '24 07:04 Dr-Blank