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

Support for working with markdown front matter #642

Merged
merged 8 commits into from
Aug 23, 2024

Conversation

pelikhan
Copy link
Member

@pelikhan pelikhan commented Aug 22, 2024

Fixes #641

Add support for working with markdown front matter.

  • packages/core/src/frontmatter.ts

    • Add splitMarkdown function to extract frontmatter and markdown content.
    • Add updateFrontmatter function to update frontmatter with new content.
    • Ensure splitMarkdown and updateFrontmatter handle various formats (yaml, json, toml).
  • packages/core/src/frontmatter.test.ts

    • Add tests for splitMarkdown function.
    • Add tests for updateFrontmatter function.
  • packages/core/src/markdown.ts

    • Import splitMarkdown and updateFrontmatter from frontmatter.ts.
    • Add mergeFrontmatter function to split markdown and update frontmatter.
    • Ensure mergeFrontmatter handles various formats (yaml, json, toml).

For more details, open the Copilot Workspace session.

  • The key changes in this GIT_DIFF focused on two areas: unit tests for various functions and changes in functionality.

  • New unit tests are added for the functions splitMarkdown and updateFrontmatter. These tests ensure that the functions work as expected with different formats of frontmatter (yaml, json, and toml) in markdown files.

  • The function splitMarkdown was introduced and it takes a string of text and an optional format (yaml, json, or toml). It divides the text into its frontmatter and content sections based on the provided format.

  • The function updateFrontmatter was introduced and it updates the existing frontmatter of a given markdown text with a new one. If no format is specified, it defaults to yaml.

  • Modifications to the frontmatterTryParse function have been made, although specific changes are slightly unclear from the diff.

  • The mergeFrontmatter function was added to the Markdown file. It combines the original frontmatter with the new one and updates it in the text, using the aforementioned splitMarkdown and updateFrontmatter functions.

  • We also see the inclusion of splitMarkdown and updateFrontmatter functions into src/markdown.ts

Key Changes:

  • 👍 More unit tests! Ensuring frontmatterTryParse, splitMarkdown, and updateFrontmatter functions are all performing well.
  • 🆕 New functions for handling frontmatter - splitMarkdown and updateFrontmatter.
  • 🔄 frontmatterTryParse function got an update, achieving more functionality.
  • ➕ Addition of these functionalities in src/markdown.ts. The power of frontmatter manipulation right in your Markdown!

Please note - no user-facing changes were made as no changes were seen in "packages/core/src/prompt_template.d.ts" and "packages/core/src/prompt_type.ts".

generated by pr-describe

Fixes #641

Add support for working with markdown front matter.

* **packages/core/src/frontmatter.ts**
  - Add `splitMarkdown` function to extract frontmatter and markdown content.
  - Add `updateFrontmatter` function to update frontmatter with new content.
  - Ensure `splitMarkdown` and `updateFrontmatter` handle various formats (yaml, json, toml).

* **packages/core/src/frontmatter.test.ts**
  - Add tests for `splitMarkdown` function.
  - Add tests for `updateFrontmatter` function.

* **packages/core/src/markdown.ts**
  - Import `splitMarkdown` and `updateFrontmatter` from `frontmatter.ts`.
  - Add `mergeFrontmatter` function to split markdown and update frontmatter.
  - Ensure `mergeFrontmatter` handles various formats (yaml, json, toml).

---

For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/microsoft/genaiscript/issues/641?shareId=XXXX-XXXX-XXXX-XXXX).
Copy link

The changes in this pull request mainly entail the implementation of functionality for parsing, splitting, and updating front matter in markdown files. The front matter can be in the format of YAML, JSON, or TOML.

  1. The function frontmatterTryParse is updated to handle parsing of frontmatter in different formats.

  2. New functions splitMarkdown and updateFrontmatter are added. splitMarkdown splits the markdown into frontmatter and content, while updateFrontmatter is used to update the frontmatter of a markdown string.

  3. Additionally, a function mergeFrontmatter is added to the markdown.ts module to merge new frontmatter with the existing frontmatter.

  4. Tests for all these functionalities have been added to frontmatter.test.ts.

The changes look good in general but I have a few concerns:

  • In the function updateFrontmatter, the switch case construct for formatting the new frontmatter may lack scalability if new formats are to be added in the future. Instead, a map or object-based approach might be preferable.
  • Error handling seems to be missing. What happens if the frontmatter format provided is not supported or if frontmatterTryParse fails? Handling these edge cases would improve the robustness of the code.
  • For the mergeFrontmatter function, it isn't clear what happens if there are duplicate keys in the new and old frontmatter. It would be beneficial to specify the behavior in such scenarios.

Apart from these, the code looks good for merging. LGTM 🚀

generated by pr-review

): string {
const { frontmatter, content } = splitMarkdown(text, options)
const updatedFrontmatter = { ...frontmatter, ...newFrontmatter }
return updateFrontmatter(text, updatedFrontmatter, options)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no null check for the splitMarkdown function result. If this function returns null or undefined, it could lead to a runtime error when trying to destructure frontmatter and content. Consider adding a null check before destructuring the result. 🛠️

generated by pr-review-commit missing_null_check

break
default:
res = YAMLTryParse(fm)
break
throw new Error(`Unsupported format: ${format}`)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default case in the switch statement throws an error for unsupported formats. However, the function does not handle this error, which could lead to unhandled exceptions. Consider providing a default format or handling this error in a way that does not interrupt the execution of the program. 😊

generated by pr-review-commit unsupported_format

frontmatterTryParse(text, { format })?.value ?? {},
content: (text) => splitMarkdown(text)?.content,
updateFrontmatter: (text, frontmatter, format): string =>
updateFrontmatter(text, frontmatter, { format }),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function updateFrontmatter is called without error handling. If this function throws an error, it could lead to an unhandled exception. Consider adding error handling to improve the robustness of your code. 😊

generated by pr-review-commit missing_error_handling


## `frontmatter`

Extracts and parses the frontmatter text from a markdown file. Returns `undefined` if no frontmatter is not found or the parsing failed. Default format is `yaml`.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be a typo here. "no frontmatter is not found" should be "no frontmatter is found".

generated by pr-docs-review-commit typo

@@ -129,7 +129,7 @@ title: "Hello, World!"
...
```

You can use the `parsers.frontmatter` to parse out the metadata into an object
You can use the `parsers.frontmatter` or [MD](./md.md) to parse out the metadata into an object

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The relative link to MD documentation should be verified to ensure it correctly points to the md.md file within the same directory.

generated by pr-docs-review-commit relative_link

text: string,
frontmatter: any,
format?: "yaml" | "json"
): string

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter descriptions for the updateFrontmatter method are missing. It would be helpful to add descriptions for text, frontmatter, and format to provide more context for developers. 😇

generated by pr-review-commit missing_param_description

@@ -129,7 +129,7 @@ title: "Hello, World!"
...
```

You can use the `parsers.frontmatter` to parse out the metadata into an object
You can use the `parsers.frontmatter` or [MD](./md.md) to parse out the metadata into an object

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The link to MD in "MD" is incorrect. It should be "MD" without the "./" since it's in the same directory.

generated by pr-docs-review-commit incorrect_link

delete frontmatter[key]
} else {
frontmatter[key] = value
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code deletes a key from the frontmatter object if the value is null. This could lead to unexpected behavior if other parts of the code rely on this key existing. Consider setting the key to null or undefined instead of deleting it.

generated by pr-review-commit delete_key

@pelikhan pelikhan merged commit 0c50d8f into main Aug 23, 2024
10 checks passed
@pelikhan pelikhan deleted the pelikhan/add-frontmatter-support branch August 23, 2024 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

helper to review and update frontmatter.
1 participant