Skip to content

Conversation

@mdbraber
Copy link
Contributor

Fixes #1402 @pjkaufman could you review?

@pjkaufman
Copy link
Collaborator

Hey @mdbraber , thanks for creating a potential fix for the issue. My concern right now is how it got as far as it did in the logic since #1402 implies that the logic actually ran for yaml key sort. The logic should not make it past this check:

const oldYaml = getYAMLText(text);
if (oldYaml === null) {
return text;
}

If there is no YAML, then it should just skip the rule as a whole. So I am more worried that this is a bandaid that hides the reason that getYAMLText is returning a non-null value for a file without YAML.

Comment on lines +66 to +69
if (doc.contents == null) {
return text;
}

Copy link
Collaborator

@pjkaufman pjkaufman Sep 24, 2025

Choose a reason for hiding this comment

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

Should this not be before line 64 since that is where the error is happening since that allows us to exit immediately after we know there is no content in the YAML? At least, that is where the error happens for me locally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes - you're right. Or should the code of

if (!yaml) {
be changed to check for an empty match by changing it to if (!yaml || yaml[1] == "")

Copy link
Collaborator

Choose a reason for hiding this comment

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

Returning an empty YAML string is fine from my understanding since it is possible to have YAML that is just empty and I would expect the content to be an empty string in that case. I am not sure if things would break if we also returned null for empty strings for the YAML.

I think that the proposed solution just before getting an empty document makes the most sense to me at this time.

@pjkaufman
Copy link
Collaborator

It has been a little a while on this. I am going to just go ahead and merge this since it looks to fix the issue.

@pjkaufman pjkaufman merged commit 7693128 into platers:master Nov 7, 2025
1 check passed
@mdbraber
Copy link
Contributor Author

Thanks Peter!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: Horizontal rule creates error with YAML parsing

2 participants