Skip to content

Conversation

@GGonnerman
Copy link

Implements #1518

@hollowhemlock
Copy link
Contributor

hollowhemlock commented Apr 10, 2025

I don't have the know how yet on how to test this but on a glance at the code:

The implementation seems to rely on adding 2 settings in the future:

    enable_multiple_template_folders: false,
    templates_folder: "",
    templates_folders: [""],

I suggest only this:

    templates_folder: "", // deprecate but leave alone if users want to revert a version they had used prior
    templates_folders: [""],

To minimize regression while minimizing too much logic, options include:

Don't worry about new user that want to revert to an older version.

Create a getter for either templates_folder: or templates_folders: to continue handling the old settings?

Migrate based off of version without destroying the data

Implement a method that checks the version and institutes a migration from templates_folder:"folder" to an array: templates_folders:["folder"]. Deprecate templates_folder but leave the data there for legacy users.

You could even implement a reversion where it migrates back to templates_folder: "", but then your going many to one. I suggest not worrying about it. The legacy setting should be fine.

This would remove the need to write logic for if enabled_multiple_folders everywhere.

@GGonnerman
Copy link
Author

Just to confirm my understanding, your recommendation would be to:

  1. Have 2 options (templates_folder and templates_folders)
  2. Have a 1-time migration that copies the value from templates_folder to templates_folders
  3. Clear the templates_folder value
  4. Rewrite the code to assume that templates_folders is used everywhere and templates_folder is deprecated

You mentioned leaving the data for templates_folder alone to support legacy users, but I don't see how to do this without issues. There doesn't seem to be a way to set a default value for a setting (that is dependent on another setting's value). Instead, if we manually check if templates_folders is empty and copy the value from templates_folder, the migration would trigger each time the plugin is launched and templates_folders is empty (which would not be expected user behavior).

Does this sound correct? Or do I have a misunderstanding somewhere?

@hollowhemlock
Copy link
Contributor

You make a good point about the re-triggering. The setting templates_folders can be undefined, it could be defaulted to a string that is for example "uninitiated_array". Then when the plugin loads check for a non existent setting or templates_folders =="uninitiated_array a setting that isn't an array.

  1. Have a 1-time migration that copies the value from templates_folder to templates_folders setting.
  2. Remove the templates_folder setting because the regression break isn't bad. I removed my personal setting "templates_folder": "settings/templates", from my data.json. Vault loaded fine. When loading my vault the template folder location setting defaulted to "templates_folder": "templates". Templater got this far:
    image
  3. Rewrite the code to assume that templates_folders is used everywhere and templates_folder is deprecated.

Disregard the first option "Don't worry about new user that want to revert to an older version" A getter that pulls from either is unnecessary.

@Zachatoo
Copy link
Collaborator

  1. Clear the templates_folder value

Why do we need to clear this? The only thing we need to check is if templates_folders is undefined, if so, copy over templates_folder. We can do this in the load_settings() method.

The setting templates_folders can be undefined, it could be defaulted to a string that is for example "uninitiated_array". Then when the plugin loads check for a non existent setting or templates_folders =="uninitiated_array a setting that isn't an array.

I recommend using undefined instead of a string, optionally with a JSDoc comment explaining what it means if it's undefined.

@hollowhemlock
Copy link
Contributor

hollowhemlock commented Apr 16, 2025

It doesn't need to be cleared.

However, I think the only reason to keep it is to revert to a prior version and maintain data. Depending on when that happens, the user could have moved the templates folder. The setting would be stale. It could silently work in the case the user is pointing to a folder that exists but is no longer in use.

Removing it would display a modal error in prior versions if the default templates folder doesn't exist.

Removing it reduces ambiguity.

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.

3 participants