fix: duplicate frontmatter (with new Obsidian API) #317
Conversation
thanks to new Obsidian API, I can readd editing with regex on the frontmatter without duplicating it
WalkthroughThe recent updates focus on enhancing the project's import structure and refining the YAML frontmatter processing. By relocating import statements and introducing a Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
package.jsonis excluded by:!**/*.jsonpnpm-lock.yamlis excluded by:!**/*.yaml
Files selected for processing (3)
- src/GitHub/upload.ts (1 hunks)
- src/conversion/compiler/embeds.ts (1 hunks)
- src/conversion/index.ts (1 hunks)
Additional comments: 3
src/conversion/index.ts (1)
- 137-150: The function
inlineTagsdemonstrates a good approach to extracting and deduplicating tags from both inline metadata and frontmatter. A few points to note:
- The use of
metadataCache.getFileCache(file)?.tagsandparseFrontMatterTags(frontmatter)to gather tags from different sources is a solid approach.- The replacement of
"/"with"_"in tags (lines 143 and 148) is consistent and prevents potential issues with tag naming conventions.- The deduplication of tags using
[...new Set([...inlineTagsInText, ...yamlTags])]is efficient and ensures that each tag is unique.This function is well-implemented and follows best practices for handling tags in YAML frontmatter.
src/conversion/compiler/embeds.ts (1)
- 25-25: The modification of the import statement for
addToYamlto import from a different location suggests a reorganization of the project structure. This change is likely part of the broader refactoring effort mentioned in the PR objectives. It's important to ensure that the new import path is correct and that theaddToYamlfunction is still accessible and functional within this file. Assuming the path has been verified and is correct, this change appears to be a straightforward adjustment to accommodate the project's evolving structure.src/GitHub/upload.ts (1)
- 15-15: The import statement for
mainConvertinghas been updated to reflect a change in the project structure. This change is consistent with the PR's objective to refactor code and improve frontmatter handling. Ensure that all references tomainConvertingthroughout the project have been updated to match this new import path to avoid any import errors.Verification successful
The review of the provided script outputs reveals that the
mainConvertingfunction is indeed defined insrc/conversion/index.tsand is imported insrc/GitHub/upload.tsusing the updated path../conversion. This confirms that the import statement formainConvertinghas been correctly updated insrc/GitHub/upload.tsto reflect the change in the project structure, as mentioned in the original review comment. Additionally, there are no indications of incorrect references tomainConvertingin other parts of the project based on the script outputs provided. Therefore, the original review comment is verified to be accurate.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all references to `mainConverting` have been updated. rg --type ts 'mainConverting' --files-with-matchesLength of output: 95
Script:
#!/bin/bash # Inspect the content of the files where `mainConverting` is referenced to verify the import paths. cat src/conversion/index.ts cat src/GitHub/upload.tsLength of output: 25912
| export function addToYaml(text: string, toAdd: string[], plugin: GithubPublisher, folderNoteParaMeters: { properties: MultiProperties | null, file: TFile}): string { | ||
| const properties = folderNoteParaMeters?.properties; | ||
| const {contentStart, exists, frontmatter} = getFrontMatterInfo(text); | ||
| if (!properties || (!properties.plugin.settings.conversion.tags.inline && !properties.plugin.settings.upload.folderNote.addTitle)) | ||
| return text; | ||
| const { settings } = plugin; | ||
| let yamlObject = parseYaml(exists ? frontmatter:"{}"); | ||
| try { | ||
| if (yamlObject && toAdd.length > 0) { | ||
| yamlObject = tagsToYaml(toAdd, settings, yamlObject); | ||
| } | ||
| if (folderNoteParaMeters?.properties) { | ||
| yamlObject = titleToYaml(yamlObject, folderNoteParaMeters.properties, folderNoteParaMeters.file); | ||
| } | ||
| if (yamlObject && Object.keys(yamlObject).length > 0) { | ||
| return `---\n${stringifyYaml(yamlObject)}---\n${(exists ? text.slice(contentStart) : text)}`; | ||
| } | ||
| } catch (e) { | ||
| new Notice(i18next.t("error.parseYaml")); | ||
| return text; //not a valid yaml, skipping | ||
| } | ||
| new Notice(i18next.t("error.parseYaml")); | ||
| return text; |
There was a problem hiding this comment.
The function addToYaml has been significantly refactored to improve the handling of YAML frontmatter. However, there are a few points to consider:
- The use of
parseYamlandstringifyYamlfrom theobsidianimport ensures proper parsing and serialization of YAML content, which is good practice. - The error handling with
try-catchblocks and the use ofnew Notice(i18next.t("error.parseYaml"));provides user feedback in case of parsing errors, enhancing the user experience. - The conditional checks and manipulation of
yamlObjectseem logical and aim to prevent duplicate tags and ensure proper YAML structure.
However, there's a potential issue with the error handling:
- The
new Notice(i18next.t("error.parseYaml"));is called twice (lines 118 and 121), once inside thecatchblock and once outside, regardless of whether an exception was thrown. This could lead to confusing user feedback since the notice will be shown even if there's no error. Consider removing the notice outside thecatchblock or adding a condition to ensure it's only shown when an actual error occurs.
- new Notice(i18next.t("error.parseYaml"));Remove or conditionally execute the second notice to avoid confusion.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| export function addToYaml(text: string, toAdd: string[], plugin: GithubPublisher, folderNoteParaMeters: { properties: MultiProperties | null, file: TFile}): string { | |
| const properties = folderNoteParaMeters?.properties; | |
| const {contentStart, exists, frontmatter} = getFrontMatterInfo(text); | |
| if (!properties || (!properties.plugin.settings.conversion.tags.inline && !properties.plugin.settings.upload.folderNote.addTitle)) | |
| return text; | |
| const { settings } = plugin; | |
| let yamlObject = parseYaml(exists ? frontmatter:"{}"); | |
| try { | |
| if (yamlObject && toAdd.length > 0) { | |
| yamlObject = tagsToYaml(toAdd, settings, yamlObject); | |
| } | |
| if (folderNoteParaMeters?.properties) { | |
| yamlObject = titleToYaml(yamlObject, folderNoteParaMeters.properties, folderNoteParaMeters.file); | |
| } | |
| if (yamlObject && Object.keys(yamlObject).length > 0) { | |
| return `---\n${stringifyYaml(yamlObject)}---\n${(exists ? text.slice(contentStart) : text)}`; | |
| } | |
| } catch (e) { | |
| new Notice(i18next.t("error.parseYaml")); | |
| return text; //not a valid yaml, skipping | |
| } | |
| new Notice(i18next.t("error.parseYaml")); | |
| return text; | |
| export function addToYaml(text: string, toAdd: string[], plugin: GithubPublisher, folderNoteParaMeters: { properties: MultiProperties | null, file: TFile}): string { | |
| const properties = folderNoteParaMeters?.properties; | |
| const {contentStart, exists, frontmatter} = getFrontMatterInfo(text); | |
| if (!properties || (!properties.plugin.settings.conversion.tags.inline && !properties.plugin.settings.upload.folderNote.addTitle)) | |
| return text; | |
| const { settings } = plugin; | |
| let yamlObject = parseYaml(exists ? frontmatter:"{}"); | |
| try { | |
| if (yamlObject && toAdd.length > 0) { | |
| yamlObject = tagsToYaml(toAdd, settings, yamlObject); | |
| } | |
| if (folderNoteParaMeters?.properties) { | |
| yamlObject = titleToYaml(yamlObject, folderNoteParaMeters.properties, folderNoteParaMeters.file); | |
| } | |
| if (yamlObject && Object.keys(yamlObject).length > 0) { | |
| return `---\n${stringifyYaml(yamlObject)}---\n${(exists ? text.slice(contentStart) : text)}`; | |
| } | |
| } catch (e) { | |
| new Notice(i18next.t("error.parseYaml")); | |
| return text; //not a valid yaml, skipping | |
| } | |
| return text; |
| function titleToYaml(yaml: any, properties: MultiProperties, file: TFile) { | ||
| const settings = properties.plugin.settings.upload.folderNote.addTitle; | ||
| if (!settings) { | ||
| return yaml; | ||
| } | ||
| if (!yaml[settings.key] && isFolderNote(properties) && settings.enable) { | ||
| yaml[settings.key] = file.basename; | ||
| } | ||
| return yaml; |
There was a problem hiding this comment.
The function titleToYaml correctly checks for the presence of a title key in the YAML frontmatter and adds it if missing, which is a useful feature for ensuring consistency in frontmatter across files. However, there's a minor improvement that could be made for clarity and maintainability:
- The condition
if (!yaml[settings.key] && isFolderNote(properties) && settings.enable)combines several checks in one line. While this is efficient, it might be beneficial for readability and future maintenance to separate these conditions, especially theisFolderNote(properties)check, which could have its result stored in a variable with a descriptive name.
Consider refactoring the condition for improved readability and maintainability.
| @@ -12,7 +12,7 @@ import { | |||
| } from "obsidian"; | |||
|
|
|||
| import { convertToHTMLSVG } from "../conversion/compiler/excalidraw"; | |||
| import { mainConverting } from "../conversion/convert_text"; | |||
| import { mainConverting } from "../conversion"; | |||
There was a problem hiding this comment.
Given the comprehensive changes across multiple files, it's crucial to ensure that the updated import paths and refactored code do not introduce any regressions or break existing functionality. Consider adding or updating unit tests to cover the changes made in this PR, especially for critical functions like publish, uploadImage, and uploadText. This will help maintain the reliability of the system as it evolves.
Would you like assistance in creating or updating the unit tests to cover these changes?
Summary by CodeRabbit