feat: prototype to use dockerfmt as fallback formatter
#432
Conversation
🦋 Changeset detectedLatest commit: 00df081 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 |
WalkthroughThe pull request integrates Dockerfile formatting into the shell plugin by introducing a fallback printer using Changes
Sequence Diagram(s)sequenceDiagram
participant U as User/Prettier Request
participant P as Parser (dockerfileParser)
participant E as Error Cache
participant D as Docker Printer (dockerPrinter)
participant F as dockerfmt Formatter
U->>P: Submit Dockerfile content (using parser: dockerfile)
P->>P: Attempt to parse using shell parser
alt Parsing fails
P->>E: Cache error details
P->>D: Trigger fallback printer
D->>F: Format content with dockerfmt
F-->>D: Return formatted output
else Parsing succeeds
P->>P: Continue normal processing
end
D-->>U: Return formatted Dockerfile output
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/test/fixtures.spec.tsOops! Something went wrong! :( ESLint: 9.24.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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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.
Copilot reviewed 7 out of 12 changed files in this pull request and generated no comments.
Files not reviewed (5)
- packages/sh/package.json: Language not supported
- packages/sh/test/snapshots/fixtures.spec.ts.snap: Language not supported
- packages/sh/test/fixtures/376.Dockerfile: Language not supported
- packages/sh/test/fixtures/384.Dockerfile: Language not supported
- packages/sh/test/fixtures/398.Dockerfile: Language not supported
Comments suppressed due to low confidence (3)
scripts/languages.ts:41
- The default assignment for 'aceModes' as [parser] may override custom passed values unexpectedly. Confirm that this aligns with the intended behavior.
aceModes: string[] = [parser],
scripts/languages.ts:129
- Removing the explicit aceModes array for 'toml' might cause inconsistencies in supported modes. Ensure that the default value produces the expected outcome.
[...getSupportedLanguages('toml')],
packages/sh/src/index.ts:9
- [nitpick] Renaming 'ShPrintOptions' to 'ShFormatOptions' may be confusing. Consider using a more descriptive alias or retaining the original name for clarity.
type ShPrintOptions as ShFormatOptions,
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #432 +/- ##
==========================================
- Coverage 88.55% 80.72% -7.83%
==========================================
Files 10 10
Lines 166 192 +26
Branches 45 51 +6
==========================================
+ Hits 147 155 +8
- Misses 18 36 +18
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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 00df081. |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (3)
packages/sh/test/fixtures/384.Dockerfile (1)
1-2:⚠️ Potential issueInstruction Order & Deprecation Warning for MAINTAINER
The Dockerfile begins with the
MAINTAINERinstruction. Note that:
- Dockerfile instructions should start with
FROM,ARG, or a comment.MAINTAINERis deprecated; it is recommended to use aLABELinstead (e.g.,LABEL maintainer="Jean Luc Picard <picardj@starfleet.gov>").If this fixture is used for testing
dockerfmtbehavior, consider adding a comment or a dummyFROMline at the very top to comply with best practices and to avoid Hadolint errors.🧰 Tools
🪛 Hadolint (2.12.0)
[error] 1-1: Invalid instruction order. Dockerfile must begin with
FROM,ARGor comment.(DL3061)
[error] 1-1: MAINTAINER is deprecated
(DL4000)
packages/sh/test/fixtures/398.Dockerfile (1)
1-8:⚠️ Potential issueDockerfile Instruction Order Issue
This Dockerfile starts directly with an
ENVinstruction, which can trigger Hadolint errors expecting aFROM,ARG, or comment at the top.
If this file is solely used as a test fixture for Dockerfile formatting, consider adding a dummyFROMor a leading comment to comply with Dockerfile best practices without affecting the intent of your tests.🧰 Tools
🪛 Hadolint (2.12.0)
[error] 1-1: Invalid instruction order. Dockerfile must begin with
FROM,ARGor comment.(DL3061)
packages/sh/test/fixtures.spec.ts (1)
37-49: 💡 Verification agent🧩 Analysis chain
Added Dockerfile-specific formatting test with proper error handling.
This section adds a new test case specifically for Dockerfile formatting using the new 'dockerfile' parser. However, there's a snapshot mismatch in the test pipeline.
🏁 Script executed:
#!/bin/bash # Check what's causing the snapshot mismatch for "147.cfg 2" # First, check if the fixture file exists ls -la packages/sh/test/fixtures/147.cfg # Then, see how it's being formatted with both parsers cat packages/sh/test/fixtures/147.cfg # Check the existing snapshot file to see what's expected find packages/sh/test -name "*.snap" -type f -exec grep -l "147.cfg 2" {} \; | xargs catLength of output: 3560
Critical: Dockerfile Test Snapshot Mismatch Needs Resolution
The new Dockerfile formatting test in
packages/sh/test/fixtures.spec.ts(lines 37–49) is still causing a snapshot mismatch for fixture147.cfg(expected snapshot labeled with"147.cfg 2"). The shell output confirms that the fixture file exists and contains the expected metadata content, yet the formatted output or caught error (formatted error message from the newdockerfileparser) does not match the stored snapshot.Please verify whether the change in output is the intended behavior. If so, update the snapshot accordingly; otherwise, adjust the formatting or error handling logic within the test to align with the expected snapshot output.
🧹 Nitpick comments (4)
test.js (1)
1-9: Top-Level Await and Error HandlingThe use of top-level
awaitis acceptable if your Node.js environment supports ES modules; ensure that the runtime is configured accordingly.
Additionally, consider adding error handling (e.g., a try-catch block) around the asynchronous call to gracefully handle any formatting errors during execution..changeset/tiny-ghosts-press.md (1)
1-6: Changeset Entry Formatting ReviewThe changeset entry clearly documents the new fallback printer feature using
dockerfmt. Double-check that the use of backticks aroundprinterandparser: dockerfileis consistent with your project’s style guidelines.packages/sh/README.md (1)
38-39: Updated documentation for ignore files limitations.The clarification about limited support for ignore files is helpful. Note that there's a minor style suggestion from static analysis.
Consider changing "can not" to "cannot" for better readability:
- We can not do much on our side. See also <https://github.com/un-ts/prettier/issues/336>. + We cannot do much on our side. See also <https://github.com/un-ts/prettier/issues/336>.🧰 Tools
🪛 LanguageTool
[style] ~39-~39: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ...cases can be handled correctly. > We can not do much on our side. See also <https://...(CAN_NOT_PREMIUM)
packages/sh/src/index.ts (1)
118-118: Potential memory growth in errorCache.
Storing parse errors in a global map could lead to unbounded memory usage if the plugin processes many files. Consider bounding or pruning this map.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/sh/test/__snapshots__/fixtures.spec.ts.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
.changeset/tiny-ghosts-press.md(1 hunks)packages/sh/README.md(2 hunks)packages/sh/package.json(1 hunks)packages/sh/src/index.ts(2 hunks)packages/sh/test/error.spec.ts(1 hunks)packages/sh/test/fixtures.spec.ts(2 hunks)packages/sh/test/fixtures/376.Dockerfile(1 hunks)packages/sh/test/fixtures/384.Dockerfile(1 hunks)packages/sh/test/fixtures/398.Dockerfile(1 hunks)packages/sh/test/parser.spec.ts(1 hunks)packages/sh/test/shim.d.ts(1 hunks)scripts/format.ts(2 hunks)scripts/languages.ts(3 hunks)test.js(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
packages/sh/test/parser.spec.ts (1)
packages/sh/src/index.ts (1)
parsers(221-224)
🪛 Hadolint (2.12.0)
packages/sh/test/fixtures/384.Dockerfile
[error] 1-1: Invalid instruction order. Dockerfile must begin with FROM, ARG or comment.
(DL3061)
[error] 1-1: MAINTAINER is deprecated
(DL4000)
packages/sh/test/fixtures/398.Dockerfile
[error] 1-1: Invalid instruction order. Dockerfile must begin with FROM, ARG or comment.
(DL3061)
🪛 GitHub Check: Lint and Test with Node.js 22 on ubuntu-latest
packages/sh/test/fixtures.spec.ts
[failure] 26-26: packages/sh/test/fixtures.spec.ts > parser and printer > should format all fixtures
Error: Snapshot parser and printer > should format all fixtures > 147.cfg 2 mismatched
❯ packages/sh/test/fixtures.spec.ts:26:58
🪛 GitHub Actions: CI
packages/sh/test/fixtures.spec.ts
[error] 26-26: Snapshot parser and printer > should format all fixtures > 147.cfg 2 mismatched
🪛 LanguageTool
packages/sh/README.md
[style] ~39-~39: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ...cases can be handled correctly. > We can not do much on our side. See also <https://...
(CAN_NOT_PREMIUM)
🪛 GitHub Check: codecov/patch
packages/sh/src/index.ts
[warning] 93-94: packages/sh/src/index.ts#L93-L94
Added lines #L93 - L94 were not covered by tests
[warning] 109-109: packages/sh/src/index.ts#L109
Added line #L109 was not covered by tests
[warning] 183-183: packages/sh/src/index.ts#L183
Added line #L183 was not covered by tests
[warning] 216-216: packages/sh/src/index.ts#L216
Added line #L216 was not covered by tests
🔇 Additional comments (34)
packages/sh/test/shim.d.ts (1)
1-8: TypeScript Declaration File – Looks GoodThe declaration for the module
'prettier-plugin-ini'is correctly defined, importing thePlugintype fromprettierand exporting the constantinias default.packages/sh/test/fixtures/376.Dockerfile (1)
1-14: Good test fixture for Dockerfile formattingThis Dockerfile demonstrates both single-line and heredoc multi-line formatting styles for the same shell commands, which makes it an excellent test case for the formatter. The use of
set -euxin both formats shows proper error handling practices in Docker scripts.packages/sh/test/parser.spec.ts (2)
4-4: Appropriate import refactoringThe direct import of
parsersaligns with the updated module structure that now exportsparsersandprintersseparately.
7-7: Updated reference to hasPragmaThis change properly references the hasPragma function through the new parsers export structure, maintaining the same functionality while simplifying the access pattern.
packages/sh/package.json (2)
53-53: Added dependency for Dockerfile formattingThe addition of
@reteps/dockerfmtprovides the necessary functionality for Dockerfile formatting. This aligns with the PR objective of using dockerfmt as a fallback formatter.
54-54: Updated sh-syntax dependencyUpgrading to
sh-syntaxversion 0.5.6 likely provides necessary support for the error handling changes seen in other files.packages/sh/test/error.spec.ts (4)
2-2: Improved error specificityThe explicit import of
ParseErrorfromsh-syntaxprovides more specific error handling capabilities.
4-4: Updated import patternThis change appropriately imports all exports from the plugin, accommodating the restructured plugin exports.
12-12: Updated plugin referenceThis change correctly updates the plugins array to use the new import name.
15-15: Enhanced error type checkingThe test now expects the more specific
ParseErrortype from sh-syntax rather than a genericSyntaxError, which better reflects the actual error handling in the updated implementation.scripts/format.ts (2)
6-7: Import statement updated to support the new module structure.The change from default imports to namespace imports for the
prettier-plugin-shmodule aligns with the PR objective of supportingdockerfmtas a fallback formatter.
21-21: Updated plugin references to match the new import structure.This change correctly utilizes the new import references for the plugins array.
packages/sh/test/fixtures.spec.ts (3)
6-6: Import statement modified to support the new module structure.Updated the import to use namespace import, which is consistent with other files in this PR.
21-22: Updated plugin reference to match the new import structure.This change correctly uses the imported namespace for the plugins array.
23-35: Enhanced error handling and conditionally process Dockerfile files.Good addition of error handling for formatting operations and conditional processing based on file type.
🧰 Tools
🪛 GitHub Check: Lint and Test with Node.js 22 on ubuntu-latest
[failure] 26-26: packages/sh/test/fixtures.spec.ts > parser and printer > should format all fixtures
Error: Snapshotparser and printer > should format all fixtures > 147.cfg 2mismatched
❯ packages/sh/test/fixtures.spec.ts:26:58🪛 GitHub Actions: CI
[error] 26-26: Snapshot
parser and printer > should format all fixtures > 147.cfg 2mismatchedpackages/sh/README.md (2)
20-36: Enhanced documentation for Dockerfile support.Good addition of detailed information about the improved Dockerfile support using
dockerfmtwith clear configuration examples.
262-262: Added link to dockerfmt repository.Good addition of the reference to the dockerfmt project.
scripts/languages.ts (3)
41-41: Added default parameter value to simplify function calls.Good enhancement by adding a default value for the
aceModesparameter, which simplifies function calls.
95-95: Updated supported modes for the 'sh' parser.The supported modes for the 'sh' parser now include 'properties' alongside 'dockerfile', 'gitignore', and 'sh', which aligns with the enhanced functionality.
129-129: Simplified function call by leveraging default parameter.The call to
getSupportedLanguagesfor 'toml' has been simplified by using the newly added default parameter value.packages/sh/src/index.ts (14)
1-2: No immediate concerns.
These imports align well with the new Dockerfile formatting functionality.
9-9: Clear renaming.
RenamingShPrintOptionstoShFormatOptionsin the import statement appears consistent with its usage throughout the file.
13-13: Exporting languages is straightforward.
Re-exporting from./languages.jsis a clean approach.
15-17: Check test coverage for DockerfilePrintOptions usage.
This interface is fairly simple, but please consider adding tests to verifyindenthandling for Dockerfiles.
19-23: Extended parser options look good.
AddingfilepathtoShParserOptionsis logical for error tracking and fallback.
25-28: Confirmed new fields for ShPrintOptions.
EnsuringfilepathandtabWidthare mandatory is appropriate.
48-87: Implementation of hasPragma is correct.
This function effectively detects@prettieror@formatin leading comments. Consider adding tests with various top-of-file scenarios to improve coverage.
89-95: Consider test coverage for the dockerfileParser fallback.
Your parser simply returns its input. Tests ensuring Dockerfile parsing logic or fallback usage are currently missing (lines 93-94).🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 93-94: packages/sh/src/index.ts#L93-L94
Added lines #L93 - L94 were not covered by tests
97-116: Add coverage for empty Dockerfile scenario.
The check for a non-existent node (line 109) is untested, which can happen if the file is empty. Consider a test that verifies we return an empty string.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 109-109: packages/sh/src/index.ts#L109
Added line #L109 was not covered by tests
120-151: Fallback to Dockerfile parse upon shell parse error.
This is a valid approach but confirm users truly intend a Dockerfile fallback for all parse failures. Tests for the fallback scenario might help confirm correctness.
153-218: Enhance test coverage for empty node and throw flow.
Lines 183 (returning''if no node) and 216 (throwing the original error) are missed by tests. Consider adding a test to trigger each path.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 183-183: packages/sh/src/index.ts#L183
Added line #L183 was not covered by tests
[warning] 216-216: packages/sh/src/index.ts#L216
Added line #L216 was not covered by tests
221-224: Exported parsers are clearly separated.
ExportingdockerfileParserandshParserdistinctly makes usage more explicit.
226-229: Exported printers are clearly separated.
Defining separate printers for Dockerfile vs. shell content seems consistent.
231-398: Options object is comprehensive.
The structured export of shell formatting options is clear. No immediate issues noted.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.size-limit.json (1)
11-13: Updated Size Limit for Dockerfile Parser ModuleThe new size limit of
"3.4kB"forpackages/sh/lib/index.jsappears to be a deliberate adjustment to accommodate the added functionality (e.g., integration of@reteps/dockerfmtas a fallback formatter) and any associated code size increase. Please verify that this revised limit meets your bundle size constraints and performance goals.packages/sh/README.md (2)
20-23: Clarify the Dockerfile fallback explanationThe updated section clearly explains that Dockerfile support now benefits from
dockerfmtas a fallback whensh-syntaxfails. Consider explicitly emphasizing that this fallback behavior only occurs on parsing failure, which might help users understand whendockerfmtis employed.
38-39: Minor language suggestion for clarityThe phrase “We can not do much on our side” would read better as “We cannot do much on our side.” This minor edit improves clarity and aligns with common style conventions suggested by static analysis.
Consider applying this diff:
- We can not do much on our side. See also <https://github.com/un-ts/prettier/issues/336>. + We cannot do much on our side. See also <https://github.com/un-ts/prettier/issues/336>.🧰 Tools
🪛 LanguageTool
[style] ~39-~39: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ...cases can be handled correctly. > We can not do much on our side. See also <https://...(CAN_NOT_PREMIUM)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/tiny-ghosts-press.md(1 hunks).size-limit.json(1 hunks)packages/sh/README.md(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .changeset/tiny-ghosts-press.md
🧰 Additional context used
🪛 LanguageTool
packages/sh/README.md
[style] ~39-~39: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ...cases can be handled correctly. > We can not do much on our side. See also <https://...
(CAN_NOT_PREMIUM)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Lint and Test with Node.js 22 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 on windows-latest
🔇 Additional comments (1)
packages/sh/README.md (1)
25-36: JSON usage example is clear and completeThe sample JSON configuration for enabling the Dockerfile parser is well formatted and helpful. It provides a practical example for users configuring their
.prettierrcfile. No changes are needed here.
|
close #384
close #376
close #278
close #429
close #398
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Documentation
Refactor & Testing
These updates provide a smoother experience when working with Dockerfiles and improve overall formatting reliability.