Conversation
|
Hi Suhendi. I quickly skimmed through the PR and it looks really good! 👍 I'll have a busy start of the week so I may have time to properly review it in the second half. |
radeklat
left a comment
radeklat
left a comment
There was a problem hiding this comment.
Once again, thank you for your PR 🙏 All my comments are only minor things, overall it's really good.
Because you opened a PR from a fork, the CI did not run for it (for security reasons) and there are some minor things reported as well. You can run it locally with poetry run delfino verify-all.
There is also no documentation for this change but I'm happy to write it once this is merged.
|
|
||
| from settings_doc import importing | ||
|
|
||
| HOOKS_PREFIX = "settings_doc_" |
There was a problem hiding this comment.
Users can specify the file they want to use. So they are not forced to combine hooks with other code, resulting in an undesirable overlap. It increases code complexity so I suggest removing it.
| @lru_cache() | ||
| def load_hooks_from_module(module: ModuleType) -> Iterable[Tuple[str, Callable]]: | ||
| collected: Set[Any] = set() | ||
| hooks: List[Tuple[str, Callable]] = [] |
There was a problem hiding this comment.
Using a dict instead of a list of tuples will allow applying the hooks directly later without iteration.
| hooks: List[Tuple[str, Callable]] = [] | |
| hooks: Dict[str, Callable] = {} |
| for name, hook in loaded_hooks: | ||
| if name == hooks.HOOK_INITIALIZE_ENVIRONMENT: | ||
| hook(env) | ||
| elif name == hooks.HOOK_PRE_RENDER: | ||
| render_kwargs = hook(dict(render_kwargs)) |
There was a problem hiding this comment.
There is a missing coverage for the else condition. There isn't one, so that makes it difficult to cover it. If you go with a Dict[str, Callable], there is no else condition. Example:
| for name, hook in loaded_hooks: | |
| if name == hooks.HOOK_INITIALIZE_ENVIRONMENT: | |
| hook(env) | |
| elif name == hooks.HOOK_PRE_RENDER: | |
| render_kwargs = hook(dict(render_kwargs)) | |
| if HOOK_INITIALIZE_ENVIRONMENT in loaded_hooks: | |
| loaded_hooks[HOOK_INITIALIZE_ENVIRONMENT](env) | |
| if HOOK_PRE_RENDER in loaded_hooks: | |
| render_kwargs = loaded_hooks[HOOK_PRE_RENDER](dict(render_kwargs)) |
| template = Environment().from_string(template) | ||
| mocker.patch("settings_doc.main.get_template", return_value=template) | ||
|
|
||
| def get_template(env: Environment, _) -> Template: |
There was a problem hiding this comment.
You couldn't have know because the coding standard I adhere to is not mentioned. But unused arguments are mentioned and explicitly deleted.
| def get_template(env: Environment, _) -> Template: | |
| def get_template(env: Environment, output_format: OutputFormat) -> Template: | |
| del output_format |
|
|
||
| from settings_doc import hooks | ||
|
|
||
| _FIXTURES_DIR = join(dirname(dirname(dirname(__file__))), "fixtures") |
There was a problem hiding this comment.
I suggest adding this to tests/constants.py and using Path instead (See an example.)
2 participants
I have finally gotten around to work on this.
I added 2 hooks
initialize_environmentandpre_render. Thepre_renderallow the user to modify therender_kwargsas necessary.Would love to know your thoughts on this.
Related Issue
Issue #9