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

Cyclomatic complexity improvement and tests for actor #1498

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

BMaxV
Copy link
Contributor

@BMaxV BMaxV commented May 7, 2023

Issue description

This is related to #1491

It's up with the goal of having smaller functions that are easier to test.

Solution description

I simply took some blocks and put them into their own functions.

Checklist

I have done my best to ensure that…

  • …I have familiarized myself with the CONTRIBUTING.md file
  • …this change follows the coding style and design patterns of the codebase
  • …I own the intellectual property rights to this code
  • …the intent of this change is clearly explained
  • …existing uses of the Panda3D API are not broken
  • …the changed code is adequately covered by the test suite, where possible.

The code is not tested, I think, but I can write tests for new functions. At least for those that I can guess at what they do, I haven't really used LODs in animations or rigs yet.

@rdb
Copy link
Member

rdb commented Aug 2, 2023

I appreciate the contribution, but I'm inclined to reject this for the reasons described in #1491. As-is there is no unit test, it increases the surface area of the API (especially since the extra methods are public), the code doesn't seem meaningfully more readable or maintainable to me with the change, and I simply don't think the perceived benefits outweigh the risks of touching the code.

@BMaxV
Copy link
Contributor Author

BMaxV commented Aug 2, 2023

As I have previously said, the animation system is "broken" in the sense that I can't reproduce the behavior the manual says should work.

Please consider how we could get to the point that the animation system works as document or to document how the current code works. I don't think it can be done without rewriting the code and/or adding tests.

The purpose of writing smaller functions is that they are smaller pieces of functionality that are easier to test.

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.

None yet

2 participants