-
Notifications
You must be signed in to change notification settings - Fork 602
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
fix(front-matter): remove os specific newline #5752
fix(front-matter): remove os specific newline #5752
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5752 +/- ##
==========================================
- Coverage 96.24% 96.23% -0.01%
==========================================
Files 481 481
Lines 38751 38749 -2
Branches 5618 5617 -1
==========================================
- Hits 37294 37290 -4
- Misses 1415 1417 +2
Partials 42 42 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reasoning makes sense to me. But this is definitely not a chore. It's either a breaking change or refactor.
const NEWLINE = `${ | ||
globalThis?.Deno?.build?.os === "windows" ? "\\r?" : "" | ||
}(?:\\n)?`; | ||
const NEWLINE = "\\r?\\n?"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const NEWLINE = "\\r?\\n?"; | |
const NEWLINE = "\\r?(?:\\n)?"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these brackets don't do anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's merge as-is but keep this suggestion in mind in case anyone runs into any issues.
I don't think this breaks any existing practical usage as the condition is now more permissive (windows-style new line is now allowed in linux and mac). How about tagging this as 'fix'? |
It's more permissive but changes expected behavior depending on the OS, no? |
If we change the parsing behavior depending on the platform, that means the users can't share the exact same markdown files between different platforms (e.g. If windows user creates a markdown file with front-matter with CRLF, then that file can't be parsed in mac/linux). I don't think that meets the expectation of the users. |
Changes
This PR removes the os specific check for
\r?
before\n
.Reasoning
This is the only occurrence where a os check is done. Even the
recognize()
function in the same module doesn't do it:std/front_matter/_shared.ts
Line 36 in 99200dc