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

Generate absolute paths #6

Closed
wants to merge 7 commits into from
Closed

Generate absolute paths #6

wants to merge 7 commits into from

Conversation

me-kell
Copy link

@me-kell me-kell commented Nov 12, 2022

@me-kell me-kell changed the title [me-kell, 2022-11-12] Generate absolute paths Generate absolute paths Nov 12, 2022
Copy link
Member

@jensens jensens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-1 here, see #5 (comment)

Copy link
Member

@jensens jensens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but would be nice to have environment handled consistently.

cookiecutter.json Show resolved Hide resolved
{%- for key, value in cookiecutter.environment|dictsort %}
{{ key }} {{ value }}
{%- endfor %}

CHAMELEON_CACHE {{ cookiecutter.environment_CHAMELEON_CACHE | abspath }}
zope_i18n_compile_mo_files {{ cookiecutter.environment_zope_i18n_compile_mo_files }}
Copy link
Member

@jensens jensens Nov 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here a feature is lost, adding arbitrary environment strings. So we should loop over all strings starting with environment_, right?

@me-kell
Copy link
Author

me-kell commented Feb 2, 2023

Are there any open questions or to dos? Can this PR be merged?

@jensens
Copy link
Member

jensens commented Feb 2, 2023

Are there any open questions or to dos? Can this PR be merged?

As mentioned in the review, there is a feature lost and we need a solution here before merge.

@me-kell
Copy link
Author

me-kell commented Feb 2, 2023

Actually I have no idea how to solve both requirements (on the one side to handle path variables and on the other side to use a generic loop for string based environment variables) without marking and knowing the type of the variable.

One workaround could be to declare the path variables (in this case "CHAMELEON_CACHE") separated from the environment variables. Thus looping through the environment and treating them as strings and declaring CHAMELEON_CACHE separately (i.e. outside of the "environment" dictionary) and treating it as path.

IMHO we pay a high price (wrong paths) for having an elegant generic handling of (at the moment) only two (actually only one if we treat CHAMELEON_CACHE separately) variables.

Any idea?

@jensens
Copy link
Member

jensens commented Jan 22, 2024

I integrated your branch with some modifications around environment handling into toward-2.0 branch.

@jensens jensens closed this Jan 22, 2024
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.

2 participants