Skip to content

Conversation

reakaleek
Copy link
Member

@reakaleek reakaleek commented May 9, 2025

Add id attribute

so instead of

---
products:
  - apm
---

use

---
products:
  - id: apm
---

This makes the model more flexible and gives us the ability to add properties in the future if requirements are added. Just in case.

@reakaleek reakaleek requested a review from a team as a code owner May 9, 2025 11:23
@reakaleek reakaleek self-assigned this May 9, 2025
@reakaleek reakaleek added the chore label May 9, 2025
@reakaleek reakaleek requested a review from Copilot May 9, 2025 11:25
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the frontmatter structure for products by introducing an id attribute instead of a simple string value.

  • Updated test cases in the YAML frontmatter tests to reflect the new mapping structure with an id key.
  • Modified the YAML deserializer in Products.cs to read and validate the new id property.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
tests/Elastic.Markdown.Tests/FrontMatter/YamlFrontMatterTests.cs Updated YAML test data to include an id attribute instead of a plain string.
src/Elastic.Markdown/Myst/FrontMatter/Products.cs Updated YAML parsing logic to extract the id mapping for products.

@reakaleek
Copy link
Member Author

Depends on the outcome of elastic/docs-content#1336 (comment)

Base automatically changed from feature/refactor-product-tags to main May 9, 2025 11:33
Copy link
Member

@Mpdreamz Mpdreamz left a comment

Choose a reason for hiding this comment

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

Can we support reading both strings and the map?

That way we can minimize the syntax until the need arrises to expand product with more properties.

I don't really see any use case now, do you have any in mind?

@reakaleek
Copy link
Member Author

reakaleek commented May 9, 2025

Can we support reading both strings and the map?

That way we can minimize the syntax until the need arrises to expand product with more properties.

I don't really see any use case now, do you have any in mind?

I also don't see an immediate reason for extending it.

Generally, I made it a habit to use properties like that in YAML because it makes it so much easier to add properties in the future, without the need to modify existing logic. Which follows the Open/Closed principle.

Also, I think it semantically makes it clearer for the user that you need to enter an ID in the list. Which removes some cognitive load.

Copy link
Member

@Mpdreamz Mpdreamz left a comment

Choose a reason for hiding this comment

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

Cool cool, I also came at it more from an authoring point of view where adding a list of keywords is easier then arrays of objects (bcuz yaml 😸)

@reakaleek reakaleek force-pushed the feature/add-id-attribute-in-parser branch from 061de89 to 32e9283 Compare May 9, 2025 16:48
@reakaleek reakaleek requested a review from Mpdreamz May 9, 2025 16:49
@reakaleek reakaleek enabled auto-merge (squash) May 9, 2025 16:59
@reakaleek reakaleek merged commit a46fbf2 into main May 9, 2025
14 checks passed
@reakaleek reakaleek deleted the feature/add-id-attribute-in-parser branch May 9, 2025 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants