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

exists should check directories #36

Open
loreanvictor opened this issue Dec 30, 2023 · 4 comments
Open

exists should check directories #36

loreanvictor opened this issue Dec 30, 2023 · 4 comments
Labels
enhancement New feature or request

Comments

@loreanvictor
Copy link
Owner

exists does not check existence of directories. this can be confusing, and also is inconsistent with behaviour of remove, where if a directory is explicitly specified (without any glob patterns) it will be removed.

originally brought up in #35 by @fwextensions.

@loreanvictor loreanvictor added the enhancement New feature or request label Dec 30, 2023
@loreanvictor
Copy link
Owner Author

loreanvictor commented Dec 30, 2023

some thoughts:

  • current behaviour is not intuitive and at least should be documented.
  • it sounds reasonable for exists to behave similarly to remove, i.e. when no glob pattern is provided then existence of directories is also checked.
  • on the other hand, while it is common use-case for remove to remove empty directories for cleanup, in most use-cases (that I can currently think of) the recipe should behave the same when some particular directory doesn't exist and where it exists but it is empty. this means, most of the time this is the correct, intended code:
    exists: dir/**/*
    and this is wrong:
    exists: dir/
    tmplr could automatically treat dir/ as dir/**/*, but that is a bit of a magical behaviour and might result in further confusion.

considering all of this, I think currently the best course of action is merely to add this behaviour to the documentation, unless use-cases for checking existence of empty directories are brought up.

@loreanvictor
Copy link
Owner Author

I added the hint to the docs. Will keep this issue open for now for further discussion on the matter.

@fwextensions
Copy link

the recipe should behave the same when some particular directory doesn't exist and where it exists but it is empty

That may often be true in practice, but I don't think it has anything to do with how exists: dir/ should behave. The current behavior seems like a bug, since it evaluates to false even if dir exists and contains files.

Directories exist whether or not they contain anything. If I just ask whether dir exists (because I don't care if it's empty or not), then a command called exists should tell me. If I care whether it's empty, I can check for dir/**/*.

Actually, I could imagine the hypothesis above being true for the remove command if it displayed an "are you sure?" confirmation. If the directory didn't exist or was empty, it could silently do its thing, while it would show the confirmation only for non-empty directories. But I don't think exists should presuppose anything about how it's being used, and should just return accurate information.

@loreanvictor
Copy link
Owner Author

I understand the logical correctness perspective. My point is, checking existence of directories seems like a foot-gun in most cases (you do it because it is easier to do and seems correct while it is not), unless I'm missing some use-cases.

A counter argument is perhaps that the current behaviour is not adequate in preventing this foot-gun? Perhaps looking at various use cases helps this discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants