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

[LiquidDoc] Modify paramDescription to be null if empty #737

Merged
merged 1 commit into from
Jan 27, 2025

Conversation

jamesmengo
Copy link
Contributor

@jamesmengo jamesmengo commented Jan 27, 2025

What are you adding in this PR?

Returns null in the liquid-html-parser if paramDescription value is ''.

Also added some test coverage in the language server for this case.

  • paramDescription is always matched in the grammar. I experimented with changing that matching logic but ultimately felt it was simpler this logic in instead stage-2, though we could revisit this
  • I included a patch bump changeset

Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@jamesmengo jamesmengo force-pushed the jm/null_param_desc branch 2 times, most recently from 6234d28 to 3cf8be9 Compare January 27, 2025 17:29
@jamesmengo jamesmengo requested a review from aswamy January 27, 2025 17:31
@jamesmengo jamesmengo marked this pull request as ready for review January 27, 2025 17:54
@jamesmengo jamesmengo requested a review from a team as a code owner January 27, 2025 17:54
@jamesmengo jamesmengo added the #gsd:44310 LiquidDoc label Jan 27, 2025
@jamesmengo jamesmengo requested a review from charlespwd January 27, 2025 17:59
@@ -1974,7 +1974,8 @@ function toHtmlSelfClosingElement(
}

function toNullableTextNode(node: ConcreteTextNode | null): TextNode | null {
if (!node) return null;
if (!node || node.value === '') return null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@charlespwd I would prefer to update the ohm logic, but it wasn't so straightforward since paramDescription is always matched.

Wanted to get a sanity check on this decision, or some help editing ohm!

Copy link
Contributor

@aswamy aswamy left a comment

Choose a reason for hiding this comment

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

The updated tests look good. Thanks for the follow-up PR. 👍

@@ -33,6 +33,7 @@ describe('Unit: makeGetLiquidDocDefinitions', () => {
@param {Number} secondParam - The second param
@param paramWithNoType - param with no type
@param paramWithOnlyName
@param {Number} paramWithNoDescription
Copy link
Contributor

Choose a reason for hiding this comment

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

you should also include a test for free-form text to see if it gets picked up by getSnippetDefinition

Copy link
Contributor Author

jamesmengo commented Jan 27, 2025

Merge activity

  • Jan 27, 2:43 PM EST: A user started a stack merge that includes this pull request via Graphite.
  • Jan 27, 2:43 PM EST: A user merged this pull request with Graphite.

@jamesmengo jamesmengo merged commit fced038 into main Jan 27, 2025
7 checks passed
@jamesmengo jamesmengo deleted the jm/null_param_desc branch January 27, 2025 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
#gsd:44310 LiquidDoc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants