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_folder() has incorrect relative logic vs generate_path() #1446

Closed
doublethefish opened this issue Sep 5, 2024 · 1 comment · Fixed by #1447
Closed

generate_folder() has incorrect relative logic vs generate_path() #1446

doublethefish opened this issue Sep 5, 2024 · 1 comment · Fixed by #1447
Labels
bug Something isn't working

Comments

@doublethefish
Copy link
Contributor

Plugin information (please complete the following information):

  • OS: OSX
  • Templater version: head
  • Obsidian version: n/a
  • Templater settings: n/a

Describe the bug
The docs and code for tp.file.folder(relative=false) are misleading/incorrect.

folder() returns a file-relative folder for relative=false and a vault-absolute path for relative=true.

This caused some weirdness in my Templated notes where:

  • Notes would sometimes (!) be created in the wrong location.
  • When notes were created in the wrong location no template (or the wrong template) would be applied.
// in file `Recurring/Daily/Journal/2024-09-05`
// A link to the following day's Daily-Journal or Daily-Note like:
[[<% tp.file.folder() %>/<% tp.date.now("YYYY-MM-DD", 1, tp.file.title, "YYYY-MM-DD") %>| ⏭️ ]]
// would resolve to:
[[Journal/2024-09-05| ⏭️ ]] // <-- sometime creating the Journal dir in the root of my vault!
// instead of:
[[Recurring/Daily/Journal/2024-09-05| ⏭️ ]] // <-- sometimes creating the Journal dir in the root of my vault!

The above snippet is imported/used/shared by multiple templates, so the folder() call is quite important./

The issue is quite obvious when you compare folder() and file() code:

// src/core/functions/internal_functions/file/InternalModuleFile.ts

    // tp.file.folder() - incorrect intended behaviour
    generate_folder(): (relative?: boolean) => string {
            // ... snip ...
            if (relative) {
                folder = parent.path;  // abs for relative 👎
            } else {
                folder = parent.name; // relative for abs 👎
            }
            // ... snip ...
    }

    // tp.file.path() - correct intended behaviour
    generate_path(): (relative: boolean) => string {
            // ... snip ...
            if (relative) {
                return this.config.target_file.path; // relative for relative👍
            } else {
                return `${vault_path}/${this.config.target_file.path}`; // abs for abs 👍
            }
            // ... snip ...
    }

Expected behaviour
relative=true to return Note-relative folder paths, and relative=false to return vault-absolute paths.

Screenshots
n/a

Additional context
Changing or "fixing" the behaviour of the folder() API would break back-compat and be a maintenance nightmare, but renaming the value and updating the docs would only affect the docs and improve clarity. However, whilst that is a best-fit for effort/reward, the API itself would be more inconsistent. One possible, and more professional option might be to create a new function dirname() (which would match pathlib and be less 'surprising' overall, or something better?) and deprecate folder(), but is it worth the effort?

@Zachatoo
Copy link
Collaborator

If we want a more consistent API, we could keep folder() as is and add a new method as you suggested, and have folder() call the new method under the hood. No one has to update their templates, and the documentation can communicate that it's deprecated or no longer recommended.

I definitely don't ever want to remove folder(), that will be a huge pain for me.

Your current PR is definitely a good enough solution, it'll mostly be developers that care about consistent APIs, and I'm guessing most Templater users are not developers.

Zachatoo added a commit that referenced this issue Sep 25, 2024
…er_docs

fix: updates docs to match _actual_ behaviour of tp.file.folder()

refs: #1446
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants