Skip to content

Refactor code and add tests#21

Open
bawahakim wants to merge 38 commits intoThe-Commit-Company:mainfrom
bawahakim:test/add-tests
Open

Refactor code and add tests#21
bawahakim wants to merge 38 commits intoThe-Commit-Company:mainfrom
bawahakim:test/add-tests

Conversation

@bawahakim
Copy link

@bawahakim bawahakim commented Jun 21, 2025

Realized the package did not have tests, so wanted to take a stab at it. This brought me down a long rabbit hole of refactoring. Hope you don't mind! I did use a bit of LLM for the refactoring, comments and boilerplate

This PR should not bring any breaking changes, as I left the original public functions that were used (either whitelist, commands, or hooks) and wrapped it with the new TypeGenerator class

Added github workflow to run the tests. I also added default linter from bench and linted the package

Note: I needed to add a new global include_custom_doctypes, otherwise it would create a test doctype in code and made cleanup a bit messy. It defaults to false to avoid breaking changes. Similarly added base_output_path but as a hidden field, which defaults to ../apps, which was necessary to keep tests clean and output generation to /tmp, also non breaking.

Questions:

  1. create_type_definition_file seems to be called on doctype update. Would it make sense to rename it update_type_definition_file? I did so in the new class
  2. create_type_definition_file has a few differences from the generate method:
  • it checks if frappe is migrating, if so returns
  • it checks if the change is coming from frappe or erpnext, if so returns
    Is there a reason to have this only in this method and not the generator methods?

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.

1 participant