mvnmgrx / kiutils

Simple and SCM-friendly KiCad file parser based on Python dataclasses for KiCad 6.0 and up.
GNU General Public License v3.0
81 stars 27 forks source link

Feature: Type annotation to support older versions of Python #22

Closed Geocali closed 2 years ago

Geocali commented 2 years ago

Made the code compatible with python 3.8. It works probably with python3.6 also, but I have not tested

Made mypy check everything correctly (added some init.py) with mypy src and mypy tests, and fixed some errors

The tests are also improved to work in my local, and with automatic test discovery

mvnmgrx commented 2 years ago

Hey there,

Thanks for your contribution. The use of the typing module makes things more concise and support for Python 3.8 or possible 3.6 is fantastic!

Before merging this PR, i would like you to quickly go over the following points that i found in your changes:

  1. In tests/test_worksheet.py as well as test.py you added some path to sys.path. Why is this neccessary? If not, please remove it.
  2. The test files test_createEmptySchematic2.expected and test_hierarchicalSchematicWithAllPrimitives2.expected do not resemble the expected output of their corresponding tests (those without the 2 in the name). Please remove said files to avoid confusion. These tests are currently known to be failing and i will work on them soon.
  3. Using a # type: ignore comment for every imported kiutils module is repeditive. It furthermore seems like this was added and then removed from only some source files. Instead, please create a mypy.ini in the repository root folder with something like:

    [mypy]
    python_version = 3.8
    
    [mypy-kiutils.*]
    ignore_missing_imports = True   
    
    [mypy-tests.*]
    ignore_missing_imports = True

    and remove the # type: ignore comments. This should suppress the warnings from mypy.

  4. The __init__.py in the src/ folder is redundant as no module files are located here, please remove it.

Looking forward to merging this PR once those points are implemented. Thanks in advance!

Best regards, M

mvnmgrx commented 2 years ago

Closed due to inactivity. Furthermore, the module structure changed and some points of the PR were implemented along side development