Feature: support snippet variable files.

[itay-raveh]

Feature: support snippet variable files.

Changes

Introduces support for reading variables from files, almost exactly like the existing feature for snippets.

Since this isn't a small PR, I tried to change as little as possible. Specifically, I mostly made another variation of each snippet-loading function for variable-loading, rather than generalizing them. Happy to refactor.

Motivation

I wanted to maintain separate folders, each with the setup for a specific workflow. This fits so well with the existing recursive snippet-file discovery, and now we'll have the same quality-of-life with variables.

Also, writing JSON in a plain <textarea> isn't fun, being able to use vscode/notepad++ is a nice bonus.

---

[artisticat1]

Thank you for the PR! I'll try to review this in the next week.

I understand you've allowed snippet variables to be written in one of three different forms. However, I feel that sticking to "${VARIABLE}": "..." may be best for the sake of simplicity/clarity — it's easy to read this as a "find-and-replace" of ${VARIABLE} with .... This is less true of VARIABLE: "..." and "VARIABLE": "...". What do you think?

[itay-raveh]

Thank you for the PR! I'll try to review this in the next week.

I understand you've allowed snippet variables to be written in one of three different forms. However, I feel that sticking to "${VARIABLE}": "..." may be best for the sake of simplicity/clarity — it's easy to read this as a "find-and-replace" of ${VARIABLE} with .... This is less true of VARIABLE: "..." and "VARIABLE": "...". What do you think?

For me, defining the variables with ${} in the name is less intuitive. It does not align with JavaScript, where a template string like ​`${VARIABLE}`​ refers to a variable named VARIABLE, not ${VARIABLE}. The three forms are just a result of making the ${} in the name optional, and the fact that JavaScript accepts both {key: "..."} and {"key": "..."} as valid and identical objects.

However, my intuition is entirely derived from preexisting JavaScript knowledge, which isn't an assumption we can make about all the plugin's users. Accordingly, I completely agree that the documentation should just stick to the single existing format, with the ${}.

In contrast, we can still have the code that handles the case with no ${}. That way, things will “just work” for users who share my intuition, or forget the ${} by mistake.

[artisticat1]

For me, defining the variables with ${} in the name is less intuitive. It does not align with JavaScript, where a template string like ​`${VARIABLE}`​ refers to a variable named VARIABLE, not ${VARIABLE}. The three forms are just a result of making the ${} in the name optional, and the fact that JavaScript accepts both {key: "..."} and {"key": "..."} as valid and identical objects.

However, my intuition is entirely derived from preexisting JavaScript knowledge, which isn't an assumption we can make about all the plugin's users. Accordingly, I completely agree that the documentation should just stick to the single existing format, with the ${}.

In contrast, we can still have the code that handles the case with no ${}. That way, things will “just work” for users who share my intuition, or forget the ${} by mistake.

That makes sense. I agree the documentation should prefer the existing ${VARIABLE} format. However, if we omit the VARIABLE: "..." and "VARIABLE": "..." formats from the documentation entirely, how will users discover them?

I suggest sticking to the existing ${VARIABLE} format in the Snippet variables section, while outlining the other two formats under a new "Snippet variable files" section for more advanced users.

Since this isn't a small PR, I tried to change as little as possible. Specifically, I mostly made another variation of each snippet-loading function for variable-loading, rather than generalizing them. Happy to refactor.

As for generalising the snippet-loading functions, yes, that would be preferable.

[artisticat1]

What's the intended behaviour when e.g. the snippet variables file is located inside the snippets folder? Or when the snippet variables folder is a subfolder of the snippets folder? I believe a common usage of the feature would include users creating a single folder as a "catch-all" for Latex Suite-related data, and storing their snippets and snippet variables there (and this is indeed what I tried to do myself). However, doing this currently raises an error.

[itay-raveh]

What's the intended behaviour when e.g. the snippet variables file is located inside the snippets folder? Or when the snippet variables folder is a subfolder of the snippets folder? I believe a common usage of the feature would include users creating a single folder as a "catch-all" for Latex Suite-related data, and storing their snippets and snippet variables there (and this is indeed what I tried to do myself). However, doing this currently raises an error.

Very good question, can't believe I didn't test this.

Below are some options we can implement to be able to determine which type of object is in a file. Tell me what you think.

Automatically with Array.isArray

Just do:

const imported = await importRaw(...);
if (Array.isArray(imported)) {
    // Parse variables
} else {
    // Parse snippets
}

This "just" fixes the bug, but it might be too "magical".

By requiring a file extension

Files that contain variables will have to be in the format (optional-file-name.)variables.js.

This allows, but also requires, explicit control by the user.

By creating a unified format

Something like:

export default {
    variables: {...}
    snippets: [...]
}

Where both keys are optional, and we aggregate all files and then parse both.

Probably overkill, just throwing it out there.

[artisticat1]

Automatically with Array.isArray

Just do:

const imported = await importRaw(...);
if (Array.isArray(imported)) {
    // Parse variables
} else {
    // Parse snippets
}

This "just" fixes the bug, but it might be too "magical".

Perhaps something along those lines, where we check whether each file

• lies inside the snippets folder only,
• lies inside the snippet variables folder only, or
• lies inside both.

We can then process each case accordingly:

• If a file lies inside the snippets folder only, process it as a snippets file.
• If a file lies inside the snippet variables folder only, process it as a snippet variables file.
• If a file lies inside both, attempt to parse it using your code snippet above.

I'm imagining something like this:

const isSnippetsFile = settings.loadSnippetsFromFileOrFolder && isInFolder(file, snippetsFolder);
const isSnippetVariablesFile = settings.loadSnippetVariablesFromFileOrFolder && isInFolder(file, snippetVariablesFolder);

// Process each case
if (isSnippetsFile && isSnippetVariablesFile) {
    ...
}
else if (isSnippetsFile) {
    ...
}
else if (isSnippetVariablesFile) {
    ...
}

This also allows us to make user notices more specific and reduce ambiguity (e.g. have "Failed to load snippets file X" and "Failed to load snippet variables file X" as separate notices, instead of a single notice "Failed to load snippet/variable file X").

[itay-raveh]

I'm imagining something like this:

const isSnippetsFile = settings.loadSnippetsFromFileOrFolder && isInFolder(file, snippetsFolder);
const isSnippetVariablesFile = settings.loadSnippetVariablesFromFileOrFolder && isInFolder(file, snippetVariablesFolder);

// Process each case
if (isSnippetsFile && isSnippetVariablesFile) {
    ...
}
else if (isSnippetsFile) {
    ...
}
else if (isSnippetVariablesFile) {
    ...
}

Where would this occur? In the onFileXXX functions?

This also allows us to make user notices more specific and reduce ambiguity (e.g. have "Failed to load snippets file X" and "Failed to load snippet variables file X" as separate notices, instead of a single notice "Failed to load snippet/variable file X").

Currently, error messages are raised from multiple places, including functions that are now generalized to handle both snippets and variables. I can try to refactor both the new and the existing code to bubble parsing errors rather than logging them. That is, until they reach a caller who knows if the error message should say "snippets" or "variables".

[artisticat1]

Where would this occur? In the onFileXXX functions?

Ah, I hadn't thought about where exactly this would occur – just what the overall logic would look like.

Here's an overview of a procedure I came up with. Let me know what you think.

1. Write a function getFilesWithin(fileOrFolder: TAbstractFile): Set that takes in a file or folder as input, collects all the TFiles within that folder, and returns them as a set.
   • (This would be similar to currently written getWithinFolder function.)
   • If fileOrFolder is a file, the set just contains that file.
2. Call getFilesWithin with the snippet file/folder location defined in the settings.
3. Call getFilesWithin with the snippet variables file/folder location defined in the settings.
4. Use set composition functions to determine which files are snippet files, variable files, or snippet-or-variable files.
   • The intersection of the sets returned in steps 2 and 3 will give us the files that lie in both the snippet and snippet variable locations.
   • The difference will give us snippet-only files and variable-only files.
5. Parse variable files and snippet-or-variable files. If a snippet-or-variable file fails to parse correctly, add it to the snippet files set.
6. Parse snippet files.

Currently, error messages are raised from multiple places, including functions that are now generalized to handle both snippets and variables. I can try to refactor both the new and the existing code to bubble parsing errors rather than logging them. That is, until they reach a caller who knows if the error message should say "snippets" or "variables".

I think the procedure I described above should solve this. (Except that at step 5/6, we may want to maintain a separate list of snippet-or-variable files that failed to parse as variable files, so if they also fail to parse as snippet files then we can print "Failed to parse snippet/variable file X")

[itay-raveh]

Generally followed this procedure, with a few minor teaks.

Set composition functions are not available in Node.js, so I installed pollyfills from es-shims

Both success and error notices are now as specific as they can be.

[artisticat1]

Works well from my testing! Looks good other than some minor formatting issues and the UI getting spammed with notices when editing snippets/variables from settings.

[itay-raveh]

Fixed all the formatting issues. Consider writing a .prettierrc file matching your style :)

As to the spam, I gave the notice a long debounce, and it's only created if at least one of them is loaded from a file.

[artisticat1]

Thanks! Yeah, I've actually looked into writing a .prettierrc file before, but I wasn't completely happy with the output and some customisation options I was looking for didn't exist. Perhaps I'll look into another formatter in future!

[artisticat1]

The upgrade to esbuild in https://github.com/artisticat1/obsidian-latex-suite/pull/280/commits/a04b33920f90d4eefced78a1926b366e0e6d116c causes the command npm run build to stall – any help? I'll revert the change temporarily so I can push out a release.

Edit: Nevermind, I appear to have solved the issue by switching between esbuild.build and esbuild.context depending on what the production flag is set to.

[itay-raveh]

The upgrade to esbuild in a04b339 causes the command npm run build to stall – any help? I'll revert the change temporarily so I can push out a release.

Edit: Nevermind, I appear to have solved the issue by switching between esbuild.build and esbuild.context depending on what the production flag is set to.

If you've solved it, then great, but I believe this upgrade can be reverted if you want.

I upgraded because the older version did not support async generators, but those are gone now. There is a single sync generator, which I believe the older version supports.

[artisticat1]

Ah, that's good to know. I suppose we might as well leave it on the more recent version though.