Fix class sorting in case of inheritance #275
Conversation
This modifies the inheritance unit test to demonstrate the sorting issue.
Replaces alphabetical sorting with topological sorting to avoid wrong class order in case of inheritance.
|
Thank you so much, @juelg! @sizmailov can you potentially merge this one? In more complex cases, it is really hard to work around the currently broken ordering of class defines 🙏 |
|
@juelg I am trying this out in AMReX-Codes/pyamrex#509 |
|
Note: not sorting classes at all might be another viable option. Declaration should will appear in the order they were registered in pybind (which presumably follow a topo sort order). The choice of sorting method depends on how one structured the binding code. I think that might deserve to be an CLI option. |
This commit introduces a new `--sort-by` CLI option to control the order of classes in the generated stub files, addressing feedback from PR sizmailov#275 and providing a more robust solution for complex projects. The available sorting strategies are: - `definition` (default): Classes are ordered as they are defined in the pybind11 module. This is achieved by iterating over `module.__dict__` instead of `inspect.getmembers`, which sorts alphabetically. This should resolve ordering issues for most standard use cases. - `topological`: Uses a topological sort based on the class inheritance hierarchy. This is useful for projects with complex inheritance structures where base classes must be defined before derived classes. Additionally, this commit adds a warning when cycles in the inheritance graph is detected, to highlight the fallback to alphabetical sorting.
|
Hi @sizmailov and @ax3l, Thank you for the feedback. I've updated the PR to add an option for the sorting behavior including topological sorting and for keeping the sorting of pybind definitions. @ax3l Can you check whether the cyclic case applies which falls back to alphabetical sorting? In the lastest version I have added a warning for this case. Does it work when you use the sorting of pybind definitions mentioned below? @sizmailov I agree and I even believe that sorting based on the definitions in the pybind file should be the default. At least, I would expect this behavior. However, when removing manual sorting in printer.py the classes stay sorted alphabetically. This is the case as What do you say? |
3 participants
Replaces alphabetical sorting with topological sorting to avoid wrong class order in case of inheritance. This PR is related to issue #231
The PR includes the commit from @ax3l in PR #238 which adds tests cases for this scenario.