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

Collected code questions and comments #68

Open
gitkief opened this issue Feb 19, 2025 · 0 comments
Open

Collected code questions and comments #68

gitkief opened this issue Feb 19, 2025 · 0 comments

Comments

@gitkief
Copy link

gitkief commented Feb 19, 2025

  1. __workflow__: Workflow | None = None,
    Why does the parameter has this dunder naming and not just workflow?

  2. lambda workflow: WorkflowDefinition.from_workflow(workflow).render(),
    At first glance, without thinking too much about it, I thought WorkflowDefinition.from_workflow(workflow).render() is already executed for the input argument workflowof the render function. So while technically correct (of course) maybe renaming the lambda argument is clearer. Similar for the snakemake renderer.

  3. ref._.parameter for ref in references if isinstance(ref, ParameterReference)
    Having a variable/attribute being reused just named _ feels awkward. Is there no better name?

  4. dewret/src/dewret/core.py

    Lines 305 to 311 in 68ef970

    return ConstructConfiguration(
    flatten_all_nested=False,
    allow_positional_args=False,
    allow_plain_dict_fields=False,
    field_separator="/",
    field_index_types="int",
    )
    Basically repeats the default arguments of the class definition . Why? In addition, in
    construct=ConstructConfiguration(), render=default_renderer_config()

    the default values for construct are fetched from the ConstructConfiguration class init as compared to the render settings. I.e. default_construct_config seems superfluous at this point.

  5. render = get_render_method(render_module, pretty=pretty)
    Could we rename the method here to render_method etc. to avoid shadowing of the main render function?

@gitkief gitkief changed the title Collected code questions Collected code questions and comments Feb 19, 2025
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

No branches or pull requests

1 participant