fix(sh): fallback to sh printer when dockerfmt throws
#455
Conversation
🦋 Changeset detectedLatest commit: 215adba The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #455 +/- ##
==========================================
- Coverage 85.71% 85.10% -0.61%
==========================================
Files 10 10
Lines 182 188 +6
Branches 47 51 +4
==========================================
+ Hits 156 160 +4
- Misses 25 27 +2
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
WalkthroughThis change introduces a fallback mechanism in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Prettier
participant dockerPrinter
participant dockerfmt
participant shPrinter
User->>Prettier: Format Dockerfile
Prettier->>dockerPrinter: print(path, options)
dockerPrinter->>dockerfmt: formatDockerfileContents(path, options)
alt dockerfmt succeeds
dockerfmt-->>dockerPrinter: formatted content
dockerPrinter-->>Prettier: formatted content
else dockerfmt throws error
dockerPrinter->>shPrinter: print(path, options)
shPrinter-->>dockerPrinter: formatted content
dockerPrinter-->>Prettier: formatted content
end
Prettier-->>User: Output formatted Dockerfile
Assessment against linked issues
Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/sh/src/index.tsOops! Something went wrong! :( ESLint: 9.26.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@1stg/eslint-config' imported from /eslint.config.js 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🔇 Additional comments (6)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
There was a problem hiding this comment.
Pull Request Overview
This PR addresses issues with dockerfmt by introducing a fallback to the sh printer when formatting a Dockerfile fails. The key changes include adding a new Dockerfile fixture (445.Dockerfile), updating corresponding snapshots, and wrapping the dockerfmt call in a try-catch to enable fallback printing with adjusted options.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/sh/test/fixtures/445.Dockerfile | New Dockerfile fixture added for testing purposes. |
| packages/sh/test/snapshots/fixtures.spec.ts.snap | Updated snapshots to reflect the changes in formatting Dockerfile output. |
| packages/sh/src/index.ts | Implemented try-catch in the dockerPrinter and introduced fallback logic. |
JounQin
changed the title
sh printer when dockerfmt throws
prettier-plugin-autocorrect
prettier-plugin-pkg
prettier-plugin-sh
prettier-plugin-sql
prettier-plugin-toml
commit: |
|
size-limit report 📦
|
|
Deploy preview for prettier ready! ✅ Preview Built with commit 215adba. |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 1ea6700 in 1 minute and 45 seconds. Click for details.
- Reviewed
110lines of code in3files - Skipped
0files when reviewing. - Skipped posting
8draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/sh/src/index.ts:139
- Draft comment:
Fallback to sh printer on dockerfmt error: ensure that using 'spaceRedirects' default value (false in try vs true in fallback) is intentional and documented. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The difference in defaults appears intentional since these are two different formatters. The code already has a detailed comment explaining the fallback behavior and linking to relevant issues. The spaceRedirects difference is an implementation detail that doesn't need additional documentation. The comment is asking for confirmation/documentation which violates our rules. Maybe the different defaults could cause unexpected behavior changes when the fallback occurs? Maybe this deserves documentation for maintainers? The code already has clear comments about the fallback behavior. The different defaults are an implementation detail that doesn't affect correctness. Users can explicitly set spaceRedirects if they care about the behavior. Delete this comment. It asks for confirmation/documentation of an implementation detail that is already adequately explained in the code comments.
2. packages/sh/src/index.ts:145
- Draft comment:
Consider logging or preserving the dockerfmt error inside the catch block for easier debugging while falling back. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The code is deliberately catching and swallowing errors from dockerfmt because it's documented as buggy (see comment). The code has an intentional fallback mechanism. Adding logging here would just add noise since we expect and handle these errors gracefully. The comment in the code already explains why errors are expected and links to relevant issues. Perhaps error logging could help debug issues with dockerfmt itself. The errors might contain useful information for the dockerfmt maintainers. The code already documents the known dockerfmt issues with links. Since this is an expected fallback path, logging errors would just add noise without providing value. The comment should be deleted. The code intentionally handles dockerfmt errors by falling back, and adding error logging would not improve the code.
3. packages/sh/test/fixtures/445.Dockerfile:1
- Draft comment:
New Dockerfile fixture added to test fallback behavior for issue Bash script in markdown file gets partially deleted #445. Verify the fixture output meets expectations. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. packages/sh/src/index.ts:139
- Draft comment:
Consider capturing the thrown error (e.g. usingcatch (error)) and logging it for better diagnostics in the fallback case. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The code intentionally handles dockerfmt errors by silently falling back to a different processor. The comment and GitHub issue links provide context for why errors are expected. Since this is a formatter/printer, logging errors would add noise without much value. The fallback behavior is the intended error handling strategy. The suggestion could help debug issues with dockerfmt. Having error logs could help identify new bugs beyond the known issues. The known bugs are already documented and linked. Adding error logging would just create noise for expected failure cases that are already handled gracefully. The comment should be deleted since the silent fallback is intentional and the error handling strategy is well documented with links to relevant issues.
5. packages/sh/src/index.ts:141
- Draft comment:
Verify the differing defaults for 'spaceRedirects' (false in the try branch vs true in the fallback) are intended. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The code shows clear intent - this is a fallback mechanism where two different formatters are used. The comment in lines 147-152 explicitly states that dockerfmt is buggy and explains why there's a fallback. The different defaults appear intentional since they're using different formatters. Asking for verification adds no value since the code and comments already show this was a deliberate choice. The comment could be pointing out a potential bug if the different defaults lead to inconsistent formatting between the two paths. The code explicitly documents this as a fallback for when dockerfmt fails, so some differences in formatting are expected and acceptable. The author has clearly thought this through. Delete the comment. It asks for verification of something that is clearly intentional and well-documented in the code.
6. packages/sh/test/__snapshots__/fixtures.spec.ts.snap:76
- Draft comment:
Typographical error: 'get_chagned_files' should likely be 'get_changed_files'. Please correct the spelling for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. packages/sh/test/__snapshots__/fixtures.spec.ts.snap:956
- Draft comment:
Typographical issue: The assignmentNAME= "ufc-web-client"contains an extra space. It is more conventional in shell scripts to writeNAME="ufc-web-client"without a space between '=' and the value. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. packages/sh/test/__snapshots__/fixtures.spec.ts.snap:25
- Draft comment:
Potential typographical error: The username 'jounqin' may be a typo. If this is unintentional, please update it to the correct spelling. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_mMLTI4RgxIWRLvMB
You can customize 
JounQin
changed the title
sh printer when dockerfmt throwssh printer when dockerfmt throws
close #441
close #445
Important
Add fallback to
shprinter indockerPrinterwhendockerfmtthrows, with new test case for verification.dockerPrinter.print()inindex.ts, add fallback toshprinter whendockerfmtthrows an error.dockerfmtby usingshprinter with the same options.445.Dockerfileinfixtures.spec.ts.snapto verify fallback behavior.445.Dockerfileadded for testing intest/fixtures.ShPrintOptionsinindex.tsto include additional printer options.This description was created by
for 1ea6700. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
Bug Fixes
Tests