Skip to content
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

Support typing #2

Merged
merged 2 commits into from
Oct 19, 2023
Merged

Support typing #2

merged 2 commits into from
Oct 19, 2023

Conversation

YishiMichael
Copy link
Contributor

Since this repository is migrated from C++, type checkers cannot perform any checking with its public interfaces. Hence I would like to add a .pyi file to support static type checking. This file is written following the module.cpp file.

Additionally, when writing the file I review some of the code as well, and I want to give some personal recommendations.

  1. Enum variables are exposed in the main namespace. This is not necessary as they are accessible under their enum classes. The issue is that JoinType and EndType both have variables Round and Square, and name collisions occur when all variables are exposed to the main namespace.
  2. FillType is named as FillRule in Clipper2 library. I would recommend following their naming style, or users learning Clipper2 from C++ documentation will be confused when finding the corresponding python interface does not match.
  3. I'm quite sure whether these functions polyTreeToPaths64, orientation, polyTreeToPaths, simplifyPath, simplifyPaths shall be exported. After trying with many strategies I still cannot figure out how I can properly passing in parameters. Moreover, from the cource code I find polyTreeToPaths64 and polyTreeToPaths are bound to the same function. I guess polyTreeToPaths is intended to be bound to PolyTreeToPathsD instead.
  4. Clipper.execute and Clipper.execute2 methods have various return types depending on returnOpenPaths, returnZ flags. I feel that there are ways to handle these flags more appropriately. Sidenote, some lines of code given in the readme playing with these flags raise error about tuple mismatching.
  5. There's a typo in the readme example file: pc.scaleFactor -> po.scaleFactor
  6. Some variables are defined using def_readwrite, some using def_property. Are there any differences?
  7. underscore_case is more preferred than camelCase when naming attributes and function parameters in python. This applies to isHole, addPath, clipType, returnOpenPath, etc.
  8. Are there any chances that execute2 returns PolyTree (it seems it always returns PolyTreeD)? I'm unsure if classes PolyPath and PolyTree should be exposed, because I cannot find how they can be obtained through clipping/offsetting operations.
  9. It's a bit unnatural to name the method returning a tree-structured result execute2. A more natural, understandable naming would be execute_tree.

All the above are my personal suggestions on the repository. Discussions are always welcome!

@drlukeparry
Copy link
Owner

Thank you for suggestion and contributions. The support for typing will be helpful when working with an IDE.

To answer your feedback and comments:

#1:
I think this can be resolved. Originally in the pybind documentation when enumerations were introduced, these appeared to only be available at the module level. Hence you have to be explicit when using in PyClipr (JoinType.Round). To get around this, a separate class will have to be assigned for each enumeration class.

#2:
This will be change to FillRule, which is an oversight and not intended.

#3:
PolyTreePaths cannot be explicitly created on purpose - it is similar with pyclipper. Due to hierarchy, it creates a tree of ownership and becomes difficult to manage ownerships, even with reference counting in PyBind and from my experience I avoided this. The only way these are created in their Integer/Float derived classes are following completion of a clipping/offset operation.

#4:
There were too many functions and options in ClipperLib and I realised it became complicated to have various cases such as the inclusion of Z attributes. Using execute & execute2 simplifies this.

#5:
Thank you for observing. Will be corrected,

#6:
Refer to pybind, def_readwrite - access and modifies the object properties directly without an intermediate function call or lambda function

#7:
This is personal taste and will not be changed.

#8:
PolyPath, PolyTree I believe these are the base classes that are used for class specialisations of the Int64 and Double precision. For simplicity, the scale factor is incorporated directly into PyClipr and the transformation is applied to covert between Int64 and floating-point precision, therefore the PolyTreeD is only returned. It doesn’t make much sense working with the integer coordinate space from my experience.

#9:
This follows the convention used previously in pyclipper. Although yes I agree, I am keeping to this for legacy purposes, however, I could offer a separated function that redirects to execute2.

@drlukeparry drlukeparry self-assigned this Oct 19, 2023
@drlukeparry drlukeparry added the enhancement New feature or request label Oct 19, 2023
@drlukeparry
Copy link
Owner

Merge commit

@drlukeparry drlukeparry merged commit b32d050 into drlukeparry:main Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants