Skip to content

Conversation

@ChekTek
Copy link
Member

@ChekTek ChekTek commented Jan 9, 2026

  • preserve manifest line endings (CRLF | LF)
  • preserve manifest indentation style (tabs | spaces)

@ChekTek ChekTek requested review from GeekyEggo and Copilot January 9, 2026 22:34
Copy link

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 enhances the pack command to preserve the original formatting of manifest files when modifying them. Previously, the manifest was always written with tab indentation and LF line endings; now it retains the original file's formatting style.

Changes:

  • Detect and preserve original line ending style (CRLF vs LF)
  • Detect and preserve original indentation style (tabs vs spaces)
  • Apply detected formatting when stringifying the modified manifest

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.


// Detect the original indentation style (tabs or spaces)
const indentMatch = original.match(/^[\t ]+/m);
const indent = indentMatch?.[0].startsWith("\t") ? "\t" : indentMatch?.[0] ?? "\t";
Copy link

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

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

The indentation detection logic is complex and unclear. If the match finds spaces, it uses all matched spaces as the indent, but if it finds tabs, it only uses a single tab. This inconsistency could lead to incorrect indentation if the original file uses multiple tabs. Consider explicitly handling the tab case to use the full matched string, or document why single tab is preferred.

Suggested change
const indent = indentMatch?.[0].startsWith("\t") ? "\t" : indentMatch?.[0] ?? "\t";
const indent = indentMatch?.[0] ?? "\t";

Copilot uses AI. Check for mistakes.

// Preserve original line endings
if (lineEnding === "\r\n") {
stringified = stringified.replaceAll("\n", "\r\n");
Copy link

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

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

Using replaceAll to convert LF to CRLF will cause issues if the stringified content already contains any CRLF sequences (which could happen if manifest values contain embedded newlines). This would result in CRCRLF sequences. Consider using replace with a regex that specifically targets LF not preceded by CR: stringified.replace(/(?<!\r)\n/g, '\r\n')

Suggested change
stringified = stringified.replaceAll("\n", "\r\n");
stringified = stringified.replace(/(?<!\r)\n/g, "\r\n");

Copilot uses AI. Check for mistakes.
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.

2 participants