-
Notifications
You must be signed in to change notification settings - Fork 34
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: Preserve YAML formatting by adding preprocessing for trailing whitespace and multiline nodes #96
base: main
Are you sure you want to change the base?
Conversation
Hi @TaylorBrennan - thanks for the PR. Can you provide an example YAML where the formatting is currently incorrect? Ideally we could add it as a test case to ensure there's no regressions in the future too. |
Hey @sethvargo - Following commit adds test cases, but for ease-of-viewing: name: "Example YAML"
description: "Example YAML with incorrect formatting"
runs:
using: "composite"
steps:
- name: Execute Tests
run: |
echo "Starting tests..."
echo "Running test suite..."
npm test
echo "Tests complete."
- name: Upload Results
uses: actions/upload-artifact@v3
with:
name: test-results
path: ./results This file in the current version will result in the runs:
using: "composite"
steps:
- name: Execute Tests
run: "echo \"Starting tests...\" \n\necho \"Running test suite...\" \nnpm test \n\necho \"Tests complete.\" \n" (Additional commit to preserve original styles, should also fix test failures 🤞🏼 ) |
NIT: I'm wondering if this is a case we should solve for given that there are appropriate linting solutions for this? https://yamllint.readthedocs.io/en/stable/rules.html#module-yamllint.rules.trailing_spaces |
I'm somewhat inclined to agree. I also think the default for most editors is "trim trailing whitespace on save" (which is why I've never personally encountered this). |
I agree that those tools & editor settings can most likely cover this for the most part (I've personally only encountered this in 1 of our workflow files). However unfortunately not all environments actually enforce the proper linting practices, in those cases when an improperly formatted file slips through it could inadvertently transform such blocks (as in my example) into something totally illegible. I think having this change as part of the codebase, while we're solving for something that there are existing methods already for, it does ensure that the tool maintains consistent and readable output - particularly for those blocks that could be negatively affected (and avoiding any unexpected surprises, that prompted me to look into this 😅 ) |
The Issue
In certain niche cases,
ratchet
fails to preserve formatting for multilinerun:
blocks.This issue occurs when the input YAML file contains hidden formatting anomalies such as:
These anomalies interfere with the YAML marshaller, causing it to serialise multiline strings as single-line strings with escaped newlines. This results in reduced readability and inconsistent output format.
The Fix
This PR introduces a preprocessing step to clean YAML files before parsing. Specifically:
This ensures that the YAML parser receives a clean, consistent file for processing.
Additionally, the
processMultilineNodes
function ensures that multiline scalar nodes (eg.run:
blocks with newlines) are explicitly set to useLiteralStyle
, preserving their formatting during serialisation.Testing
This fix was validated by:
run:
blocks containing special characters, variables, and command substitutions.Impact
This fix ensures proper formatting of YAML files without requiring manual intervention.
Previously, the issue could be resolved by preprocessing files with:
Now, the preprocessing is performed natively within the too, eliminating the need for external commands or tools.