feat: make dockerfile
8000
default, no fallback needed
#436
Conversation
🦋 Changeset detectedLatest commit: c00ad78 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 |
WalkthroughThis pull request introduces a new changeset for the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CLI as Prettier CLI
participant Plugin as prettier-plugin-sh
participant Lang as getSupportedLanguages
participant Parser as shParser
participant Printer as shPrinter
U->>CLI: Submit file for formatting
CLI->>Plugin: Delegate formatting task
Plugin->>Lang: Request supported languages (parser='dockerfile')
Lang-->>Plugin: Return language details
Plugin->>Parser: Parse file content as Node
Parser-->>Plugin: Return parsed Node
Plugin->>Printer: Format parsed Node
Printer-->>Plugin: Return formatted code
Plugin-->>CLI: Deliver formatted output
CLI-->>U: Output formatted file
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/loc-functions.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 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 (2)
🪧 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 (
|
There was a problem hiding this comment.
Copilot reviewed 5 out of 8 changed files in this pull request and generated no comments.
Files not reviewed (3)
- packages/sh/package.json: Language not supported
- packages/sh/test/snapshots/fixtures.spec.ts.snap: Language not supported
- packages/sh/test/fixtures/no-ext: Language not supported
|
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. |
prettier-plugin-autocorrect
prettier-plugin-pkg
prettier-plugin-sql
prettier-plugin-sh
prettier-plugin-toml
commit: |
size-limit report 📦
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #436 +/- ##
==========================================
+ Coverage 84.42% 86.18% +1.76%
==========================================
Files 10 10
Lines 199 181 -18
Branches 52 46 -6
==========================================
- Hits 168 156 -12
+ Misses 30 24 -6
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
📊 Package size report -0.07%↓
Unchanged files
🤖 This report was automatically generated by pkg-size-action |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/sh/README.md (1)
19-21: Simplified warning section.The warning section has been streamlined to focus only on
ignorefiles limitations now thatdockerfileis the default option.Consider using "cannot" instead of "can not" in line 21 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] ~21-~21: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ...ge 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)
147-153: Error handling removed from parse methodThe error handling logic has been removed from the
parsemethod. The previous implementation likely had a try-catch block that would catch errors and provide a fallback mechanism.This change aligns with making
dockerfilethe default parser with no fallback needed, but ensure the code properly propagates errors to callers instead of silently failing.Consider adding some basic error handling or at least ensuring errors are properly propagated with clear messages:
return processor(text, { filepath, keepComments, variant, stopAt, recoverErrors, + onError: (err) => { + throw new ShSyntaxParseError(err); + } })
📜 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 (7)
.changeset/eight-apricots-give.md(1 hunks)packages/sh/README.md(2 hunks)packages/sh/package.json(1 hunks)packages/sh/src/index.ts(3 hunks)packages/sh/test/fixtures.spec.ts(1 hunks)packages/sh/test/fixtures/no-ext(1 hunks)scripts/languages.ts(2 hunks)
🧰 Additional context used
🪛 LanguageTool
packages/sh/README.md
[style] ~11-~11: The phrase ‘a lot of’ might be wordy and overused. Consider using an alternative.
Context: ... account. This plugin adds support for a lot of files through [mvdan-sh][] via [sh-synt...
(A_LOT_OF)
[style] ~21-~21: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ...ge 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] 133-134: packages/sh/src/index.ts#L133-L134
Added lines #L133 - L134 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Lint and Test with Node.js 22 on windows-latest
🔇 Additional comments (9)
.changeset/eight-apricots-give.md (1)
1-6: LGTM!The changeset file correctly documents the feature change, making
dockerfilethe default option without needing fallback.packages/sh/test/fixtures/no-ext (1)
1-1: Fixed shebang format.Good correction of the shebang line from
#/usr/bin/env shto#!/usr/bin/env sh, which ensures proper shell script recognition by the operating system.packages/sh/package.json (1)
53-53:✅ Verification successful
LGTM! Dependency version updated.
The version bump of
@reteps/dockerfmtfrom^0.2.5to^0.2.6aligns with makingdockerfilethe default parser option.
🏁 Script executed:
#!/bin/bash # Verify if this is the latest stable version of the dependency curl -s https://registry.npmjs.org/@reteps/dockerfmt | jq '.["dist-tags"].latest'Length of output: 91
Dependency Update Verified
The update of
@reteps/dockerfmtfrom^0.2.5to^0.2.6has been confirmed against the npm registry, which reports "0.2.6" as the latest stable version. This aligns with usingdockerfileas the default parser option. LGTM!packages/sh/README.md (1)
11-11: Improved clarity about supported file types.The updated description more precisely indicates how the plugin supports various file types through the [sh-syntax] and [dockerfmt] libraries.
🧰 Tools
🪛 LanguageTool
[style] ~11-~11: The phrase ‘a lot of’ might be wordy and overused. Consider using an alternative.
Context: ... account. This plugin adds support for a lot of files through [mvdan-sh][] via [sh-synt...(A_LOT_OF)
scripts/languages.ts (2)
39-41: Updated type signature to include 'dockerfile'.Good addition of 'dockerfile' to the supported parser options in the type signature.
92-97: Made 'dockerfile' a first-class parser.The code now correctly handles 'dockerfile' as a separate parser by:
- Including languages from
getSupportedLanguages('dockerfile')- Removing 'dockerfile' from the aceModes array for the 'sh' parser
This aligns with making 'dockerfile' the default option as stated in the changeset.
packages/sh/test/fixtures.spec.ts (1)
17-17:✅ Verification successful
Test simplified to handle all fixtures without parser-specific conditions
The test case has been simplified to run formatting on all fixture files without conditionally specifying parsers based on file type (like checking if a file is named Dockerfile). This aligns with making
dockerfilethe default parser, but ensure the test suite still provides adequate coverage for both shell scripts and Dockerfiles.
🏁 Script executed:
#!/bin/bash # Check if there are both shell script and Dockerfile fixtures in the test directory fixtures_dir="packages/sh/test/fixtures" echo "Checking test fixtures directory for different file types:" ls -la "$fixtures_dir" echo -e "\nCounting shell scripts and Dockerfiles:" shell_count=$(find "$fixtures_dir" -name "*.sh" | wc -l) dockerfile_count=$(find "$fixtures_dir" -name "Dockerfile*" | wc -l) echo "Shell scripts: $shell_count" echo "Dockerfiles: $dockerfile_count" if [ "$shell_count" -eq 0 ] || [ "$dockerfile_count" -eq 0 ]; then echo "WARNING: Missing test fixtures for one of the supported file types" fiLength of output: 1716
Test Fixtures Coverage Verified
The updated test case now formats all fixture files uniformly without relying on parser-specific conditions. As confirmed by the verification script, the fixtures directory contains 5 shell scripts and 1 Dockerfile, ensuring that both file types remain well-covered after setting
dockerfileas the default parser.packages/sh/src/index.ts (2)
189-206: Simplified printer implementationThe printer implementation has been simplified to directly process the node as a
Filewithout checking for alternative types or using fallback mechanisms. This is consistent with the changes to the parser and aligns with makingdockerfilethe default.
130-135:❓ Verification inconclusive
Simplified parser type signature and location functions
The parser type has been updated from
Parser<Node | string>toParser<Node>, and the location functions have been simplified to directly access Node properties without type checking for strings. This removes the string fallback handling, making the code cleaner but potentially less robust for edge cases.
🏁 Script executed:
#!/bin/bash # Check test coverage for the modified location functions echo "These location functions (lines 133-134) are not covered by tests according to codecov." echo "Checking if there are any existing tests that exercise the parser location functions:" rg -l "locStart|locEnd" packages/sh/testLength of output: 408
Action required: Please manually verify the new location functions
The parser refactor now simplifies the type signature (from
Parser<Node | string>toParser<Node>) and accesses node locations directly vianode.Pos.Offsetandnode.End.Offset, removing string fallback handling. However, based on the search inpackages/sh/test, these location functions aren’t exercised by any tests, so their robustness in edge cases—especially those previously handled by string logic—remains unverified.
- Verify: Manually test scenarios where nodes might previously have been strings.
- Consider: Adding tests to cover the behavior of
locStartandlocEndfunctions.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 133-134: packages/sh/src/index.ts#L133-L134
Added lines #L133 - L134 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/sh/README.md (2)
11-11: Good enhancement to the plugin description.The updated description clearly specifies the technology stack supporting the plugin, making it easier for users to understand its capabilities.
Consider being more specific than "a lot of files" for a more professional tone. Something like "various file formats" or "multiple file types" would be more precise.
🧰 Tools
🪛 LanguageTool
[style] ~11-~11: The phrase ‘a lot of’ might be wordy and overused. Consider using an alternative.
Context: ... account. This plugin adds support for a lot of files through [mvdan-sh][] via [sh-synt...(A_LOT_OF)
20-21: Documentation streamlined with new default behavior.The warning section has been appropriately simplified now that
dockerfileis the default option. This aligns well with the PR's objective.Minor grammar suggestion: "can not" should be "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] ~21-~21: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ...ge 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 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 (8)
.changeset/eight-apricots-give.md(1 hunks)packages/sh/README.md(2 hunks)packages/sh/package.json(1 hunks)packages/sh/src/index.ts(3 hunks)packages/sh/test/error.spec.ts(0 hunks)packages/sh/test/fixtures.spec.ts(1 hunks)packages/sh/test/fixtures/no-ext(1 hunks)scripts/languages.ts(2 hunks)
💤 Files with no reviewable changes (1)
- packages/sh/test/error.spec.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- .changeset/eight-apricots-give.md
- packages/sh/test/fixtures/no-ext
- packages/sh/package.json
- scripts/languages.ts
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/sh/src/index.ts
[warning] 133-134: packages/sh/src/index.ts#L133-L134
Added lines #L133 - L134 were not covered by tests
🪛 LanguageTool
packages/sh/README.md
[style] ~11-~11: The phrase ‘a lot of’ might be wordy and overused. Consider using an alternative.
Context: ... account. This plugin adds support for a lot of files through [mvdan-sh][] via [sh-synt...
(A_LOT_OF)
[style] ~21-~21: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ...ge 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 (5)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Lint and Test with Node.js 20 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 on 6D47 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 (4)
packages/sh/test/fixtures.spec.ts (1)
17-17: Test simplification aligns with making dockerfile the default.The test case has been generalized to handle all fixtures without special conditions, which properly reflects the change to make
dockerfilethe default parser without fallbacks.packages/sh/src/index.ts (3)
130-130: Type signature properly narrowed.The type signature has been updated from
Parser<Node | string>toParser<Node>, which is more precise and removes the string fallback, aligning with the PR's objective to makedockerfilethe default without fallbacks.
147-154: Error recovery logic removed.The error handling and fallback to the original text has been removed, which is consistent with making
dockerfilethe default option. This simplifies the code but means errors will now propagate directly to the caller.
189-206: Streamlined processing in shPrinter.The conditional check for node type has been removed, and the function now directly processes the node as a File. This simplification is appropriate given the type narrowing earlier in the code.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/sh/test/loc-functions.spec.ts (4)
9-22: Well-structured test with clear assertions.This test appropriately verifies the basic functionality of the location functions with a simple shell script. The comments before each assertion improve readability.
However, consider adding more specific tests for the shebang line itself, to ensure it's properly parsed.
+ it('should correctly handle shebang line', async () => { + const script = '#!/bin/sh\necho "test"' + const node = await parsers.sh.parse(script, { + filepath: 'test.sh', + locStart, + locEnd, + } as ParserOptions<Node>) + + // Check that we can access the first command (echo) + const command = node.commands?.[0] + expect(command).toBeDefined() + // The command should start after the shebang line + expect(locStart(command)).toBeGreaterThan(8) + })
24-47: Good coverage of different shell script constructs.This test effectively verifies that location functions work with various shell syntax constructs.
Consider two improvements:
- Add more complex shell script constructs (nested structures, loops, command substitutions)
- Split into individual test cases for better failure isolation
- it('should handle scripts with various syntax constructs', async () => { - const scripts = [ - // Simple command - 'echo "simple"', - - // If statement - 'if [ "$1" = "test" ]; then\n echo "test"\nfi', - - // Function definition - 'function say_hello() {\n echo "Hello"\n}', - ] - - for (const script of scripts) { - const node = await parsers.sh.parse(script, { - filepath: 'test.sh', - locStart, - locEnd, - } as ParserOptions<Node>) - - expect(locStart(node)).toBe(0) - expect(locEnd(node)).toBe(script.length) - } - }) + it('should handle simple commands', async () => { + const script = 'echo "simple"' + const node = await parsers.sh.parse(script, { + filepath: 'test.sh', + locStart, + locEnd, + } as ParserOptions<Node>) + + expect(locStart(node)).toBe(0) + expect(locEnd(node)).toBe(script.length) + }) + + it('should handle if statements', async () => { + const script = 'if [ "$1" = "test" ]; then\n echo "test"\nfi' + const node = await parsers.sh.parse(script, { + filepath: 'test.sh', + locStart, + locEnd, + } as ParserOptions<Node>) + + expect(locStart(node)).toBe(0) + expect(locEnd(node)).toBe(script.length) + }) + + it('should handle function definitions', async () => { + const script = 'function say_hello() {\n echo "Hello"\n}' + const node = await parsers.sh.parse(script, { + filepath: 'test.sh', + locStart, + locEnd, + } as ParserOptions<Node>) + + expect(locStart(node)).toBe(0) + expect(locEnd(node)).toBe(script.length) + }) + + it('should handle more complex constructs', async () => { + // Add test for a more complex example, e.g. with loops and nesting + const script = 'for i in $(seq 1 3); do\n if [ "$i" -eq 2 ]; then\n echo "Found $i"\n fi\ndone' + const node = await parsers.sh.parse(script, { + filepath: 'test.sh', + locStart, + locEnd, + } as ParserOptions<Node>) + + expect(locStart(node)).toBe(0) + expect(locEnd(node)).toBe(script.length) + })
10-15: Consider extracting a helper function for parsing.The call to
parsers.sh.parseis duplicated across tests. Consider extracting a helper function to reduce duplication.+ const parseScript = async (script: string) => { + return parsers.sh.parse(script, { + filepath: 'test.sh', + locStart, + locEnd, + } as ParserOptions<Node>) + } + it('should return correct offsets for simple shell scripts', async () => { const script = '#!/bin/sh\necho "Hello, world!"' - const node = await parsers.sh.parse(script, { - filepath: 'test.sh', - locStart, - locEnd, - } as ParserOptions<Node>) + const node = await parseScript(script)
36-41: Tests only basic node positions.The current tests only verify the root node's position. Consider testing specific node positions within the AST to ensure nested structures are correctly located.
for (const script of scripts) { const node = await parsers.sh.parse(script, { filepath: 'test.sh', locStart, locEnd, } as ParserOptions<Node>) expect(locStart(node)).toBe(0) expect(locEnd(node)).toBe(script.length) + + // Test positions of child nodes when available + if (node.commands && node.commands.length > 0) { + const firstCommand = node.commands[0] + expect(locStart(firstCommand)).toBeGreaterThanOrEqual(0) + expect(locEnd(firstCommand)).toBeLessThanOrEqual(script.length) + expect(locEnd(firstCommand)).toBeGreaterThan(locStart(firstCommand)) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/sh/README.md(2 hunks)packages/sh/test/loc-functions.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/sh/README.md
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/sh/test/loc-functions.spec.ts (1)
packages/sh/src/index.ts (1)
parsers(210-213)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Lint and Test with Node.js 18 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 on windows-latest
🔇 Additional comments (2)
packages/sh/test/loc-functions.spec.ts (2)
1-5: Good import organization.The imports are well-structured, separating type imports from module imports with a blank line.
6-8: Clear test suite setup.The test suite is well-organized, and extracting the location functions directly in the describe block makes them easily accessible for all tests.
|
Summary by CodeRabbit