tera icon indicating copy to clipboard operation
tera copied to clipboard

globwalk does not work with several relative paths

Open FibreFoX opened this issue 4 years ago • 2 comments

While migrating from a NodeJS-based project to a Rust-based project I found Tera as a template-engine, it was ment to be my introduction project to learn Rust.

As I separate my program and the used templates, I wanted to pass a relative path of some upper folder, but Tera did not find them. After some investigation (and local experiments to change globwalk with glob, which did not work due to unsupported syntax having curly-braces "examples/basic/templates/**/*.{html, xml}"), I found out that it only is an issue for path that point to the current or upper-level folders.

Globwalk does not work for paths starting with./ or ../, there is already an issue open at the other project: https://github.com/Gilnaa/globwalk/issues/28

FibreFoX avatar Nov 14 '20 09:11 FibreFoX

So it should be fixed upstream then

Keats avatar Nov 14 '20 12:11 Keats

At least there, but until then I think it's worth to get mentioned somewhere, that's why I opened this issue here. Others might wonder too.

FibreFoX avatar Nov 14 '20 15:11 FibreFoX

Hi, due to https://github.com/Gilnaa/globwalk/issues/28 mentioned above passing a path starting with ./ or ../ to Tera::new is completely broken. It does not look like https://github.com/Gilnaa/globwalk/issues/28 is going to be fixed any time soon (it's been 2 years already and there is no consensus on the direction to take).

Could Tera maybe side-step the issue by switching to glob or globset?

In my project I'm working around this by taking care to normalize the path I pass to Tera::new, but if there is a clear direction for a fix I could maybe contribute it.

eguiraud avatar Jan 20 '23 22:01 eguiraud

Could Tera maybe side-step the issue by switching to glob or globset?

If we can make it work without breaking any existing code, sure.

Keats avatar Jan 21 '23 19:01 Keats

without breaking any existing code

I am not sure how to guarantee that (I am not sure globwalk and globset behave identically for all inputs).

Something simpler to check would be that the change doesn't break any existing test :sweat_smile:

eguiraud avatar Jan 21 '23 20:01 eguiraud

Alternatively we could check if the input starts with ./ or ../ and return an error instead of an empty list and document this for the Tera::new command.

This wouldn't really fix the issue, but at least make it more visible why tera doesn't work as expected for this usecase.

caemor avatar Jan 22 '23 13:01 caemor

That would still be a breaking change though. If someone can do a PR we can test things.

Keats avatar Jan 22 '23 17:01 Keats

we could check if the input starts with ./ or ../ and return an error

That would still be a breaking change though.

Only for users that expect that adding a ./ at the beginning of the path always results in an empty list. I'm not sure why you would want to keep those weirdos happy... :smile:

If someone can do a PR we can test things.

It should be easy enough to have Tera detect this case and error out, I'll try to propose a PR soon. Too bad there is no clear path to an actual resolution.

eguiraud avatar Jan 23 '23 20:01 eguiraud

I'm not sure why you would want to keep those weirdos happy... 😄

https://xkcd.com/1172/

Keats avatar Jan 24 '23 14:01 Keats

I'm perfectly sure those weirdos exist (hyrum's law and all that) :smile: but you don't necessarily have to keep them happy :P

eguiraud avatar Jan 24 '23 18:01 eguiraud

Yeah it's probably fine if it always returned an empty list of files tbh

Keats avatar Jan 24 '23 19:01 Keats

A PR that makes Tera::new error out is up. Locally it produced no test failures.

For the record I still think that doing what 99.9% of users would expect out of the box (i.e. treating ./some/dir/*.html the same as /path/to/some/dir/*.html) would be better, but I guess that's a bigger change in behavior.

eguiraud avatar Jan 24 '23 20:01 eguiraud

Not sure that I am happy with Tera reporting errors with relative paths like in that PR. It does not fix the problem, it just makes more noise. In my opinion it would be a better fix to use something different than globwalk (and yes, breaking changes are okay IMHO if they introduce fixes).

FibreFoX avatar Jan 25 '23 06:01 FibreFoX

Can we do a PR with a fix as well?

Keats avatar Jan 26 '23 14:01 Keats

I agree with you @FibreFoX , I just thought there was no consensus on what an actual fix should/would look like, so in the meanwhile I'd rather have Tera report the issue rather than silently having a surprising behavior.

@Keats a PR with what I would propose as an actual fix is at https://github.com/Keats/tera/pull/799 .

eguiraud avatar Jan 26 '23 17:01 eguiraud