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

Add an OS specific helper for replacing characters for file separators #692

Closed
jkleve opened this issue Jun 8, 2022 · 13 comments · Fixed by #932
Closed

Add an OS specific helper for replacing characters for file separators #692

jkleve opened this issue Jun 8, 2022 · 13 comments · Fixed by #932
Labels
enhancement good first issue Easy things for newbies hacktoberfest-accepted This PR is a candidate to win a T-shirt if done while the Hacktoberfest is live help wanted The issue is valid, but we need community contributions to fix it
Milestone

Comments

@jkleve
Copy link

jkleve commented Jun 8, 2022

From a discussion we had about creating a directory structure, it was proposed to use replace(".", "/") in a task script. As this is specific to unix like filesystems it would be nice to have a helper that is OS specific. On Windows it will be \ and on unix like OS' it'll be /.

The working example (unix specific) is

# task.sh.jinja
templates_dir=templates
src_dir=src/main/{{ package.replace(".", "/") }}
test_dir=src/test/{{ package.replace(".", "/") }}

mkdir -p "$src_dir"
mkdir -p "$test_dir"

A nice alternative, as proposed by @yajo & @pawamoy would be

# task.sh.jinja
templates_dir=templates
src_dir=src/main/{{ package.replace(".", copier_sep) }}
test_dir=src/test/{{ package.replace(".", copier_sep) }}

mkdir -p "$src_dir"
mkdir -p "$test_dir"
@pawamoy
Copy link
Contributor

pawamoy commented Jun 8, 2022

I don't think the distinction between Linux and Windows is relevant: indeed, the point of this issue is to provide a way to create directory structures without tasks, and the suggestion is to provide a context variable (for example copier_sep or os_sep) that is not a real OS separator (because they are not allowed in filenames), and that Copier will handle as if it was an actual separator. So in any case, users will use a single variable, whatever the OS is, and Copier will run Python's mkdir, where / works on all systems anyway.

@jkleve
Copy link
Author

jkleve commented Jun 8, 2022

I see. You were pushing to add a feature where we can do this as a task in copier.yml instead of in a shell script?

@pawamoy
Copy link
Contributor

pawamoy commented Jun 8, 2022

IIUC, what @yajo suggested is that Copier natively supports creating arbitrarily deep directories without tasks, yes, by allowing writers to use a copier_sep/os_sep constant in their filenames.

@yajo yajo added good first issue Easy things for newbies hacktoberfest-accepted This PR is a candidate to win a T-shirt if done while the Hacktoberfest is live help wanted The issue is valid, but we need community contributions to fix it labels Jun 9, 2022
@yajo yajo added this to the Later milestone Jun 9, 2022
@rafalkrupinski
Copy link
Contributor

umm... this works for me. You can use forward slash on Windows, no problem.
I just made a custom jinja extension with a global DIR_SEP='/' and Copier creates directory structure.

@rafalkrupinski
Copy link
Contributor

Oh, and I had to add one line to copier, as it doesn't create parent directories for rendered files.
I'll prepare a patch

@rafalkrupinski
Copy link
Contributor

In the PR 6e06df7 there's a test for this behaviour. It passes sep: '/' as part of data

@rafalkrupinski
Copy link
Contributor

@yajo @pawamoy ,
Do you think Copier should provide a separator variable to facilitate this?

@pawamoy
Copy link
Contributor

pawamoy commented Jan 21, 2023

Yes definitely. And it makes sense that it doesn't need to be a special character, since filenames will use the variable and not the character (slash) itself :)

@rafalkrupinski
Copy link
Contributor

Yes definitely. And it makes sense that it doesn't need to be a special character, since filenames will use the variable and not the character (slash) itself :)

Introducing a global variable would break existing templates for anyone who already uses that name, regardless of what name we pick. So it would be a breaking change, unless there's a configuration variable:
copier.yaml:

_separator_key: os_sep

WDYT?

@pawamoy
Copy link
Contributor

pawamoy commented Jan 21, 2023

I think the risk of clashing with the name of an answer in someone's template is very low, especially if we prefix it with an underscore. But I'll let @yajo decide 🙂

@rafalkrupinski
Copy link
Contributor

I think the risk of clashing with the name of an answer in someone's template is very low, especially if we prefix it with an underscore. But I'll let @yajo decide 🙂

Ah, I didn't think of adding a prefixed variable. Yeah, that works.

In that case, how about exposing a subset of os as _os? I can think os.environ might be useful. Either way'd we have a "hook" for future os-specific extensions.

@pawamoy
Copy link
Contributor

pawamoy commented Jan 21, 2023

Ha, very interesting idea 🤔

@yajo
Copy link
Member

yajo commented Jan 26, 2023

I can think os.environ might be useful

There's https://pypi.org/project/jinja2-getenv-extension/ so we shouldn't need this.

@yajo yajo linked a pull request Jan 26, 2023 that will close this issue
yajo pushed a commit that referenced this issue Jan 27, 2023
…` to allow generating dynamic directory structures

* feat(Worker): make sure the parent directory of the target file exists before writing to it
* feat(_copier_conf): add directory separator (os.sep) to _copier_conf

Fix #692
@yajo yajo closed this as completed in #932 Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement good first issue Easy things for newbies hacktoberfest-accepted This PR is a candidate to win a T-shirt if done while the Hacktoberfest is live help wanted The issue is valid, but we need community contributions to fix it
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants