Skip to content

Add Stylua Support for Lua Formatting #5

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

Merged
merged 3 commits into from
Jul 20, 2025
Merged

Conversation

GowayLee
Copy link
Owner

This PR adds full Stylua support to reform.nvim, replacing the previous Python formatter fallback for Lua files.

Changes

  • Created new Stylua formatter module (lua/reform/formatters/stylua.lua)
  • Updated formatter registry to use Stylua for Lua files
  • Updated configuration defaults to include Stylua for Lua
  • Updated documentation with Stylua requirements and usage

Benefits

  • Proper Lua code formatting using dedicated formatter
  • Maintains existing functionality and API
  • Follows established patterns in the codebase

Fixes #4


🤖 Generated with Claude Code

- Create new Stylua formatter module following existing patterns
- Update formatter registry to use Stylua for Lua files
- Update configuration defaults to include Stylua for Lua
- Update README documentation to mention Stylua requirements

This replaces the previous fallback to Python formatter (autopep8) for Lua files
with proper Stylua formatting, which is more appropriate for Lua code.

Fixes #4

Co-authored-by: Li Haoyuan <[email protected]>
Copy link
Contributor

claude bot commented Jul 19, 2025

Claude finished @GowayLee's task —— View job


I'll analyze this and get back to you.

@GowayLee GowayLee force-pushed the claude/issue-4-20250719-1912 branch from 136aa0b to 50d8d30 Compare July 20, 2025 02:46
Repository owner deleted a comment from claude bot Jul 20, 2025
Repository owner deleted a comment from claude bot Jul 20, 2025
@GowayLee
Copy link
Owner Author

GowayLee commented Jul 20, 2025

Code Review: Stylua Support Implementation

Overview The PR successfully adds Stylua support for Lua formatting, replacing the Python formatter fallback. The implementation follows existing patterns and maintains backward compatibility.

Detailed Review

  1. Stylua Formatter Implementation (lua/reform/formatters/stylua.lua)
    ✅ Strengths:

Follows established patterns: Inherits from BaseFormatter correctly
Type annotations: Good use of Lua type annotations
Availability caching: Implements smart caching for is_available() checks
Error handling: Proper error handling and fallback behavior
⚠️ Areas for Improvement:

a) Security concern - Command injection vulnerability:

-- Current implementation (stylua.lua:50)
local cmd = "stylua -"
local result = vim.fn.system(cmd, text)
Issue: Direct shell command execution without proper escaping. While Stylua is a trusted formatter, this could be problematic if extended later.

b) Missing Stylua configuration support:

No way to pass Stylua configuration options
No support for .stylua.toml detection or custom configuration paths
c) Insufficient error reporting:

When formatting fails, the original text is returned silently
Users won't know why formatting didn't work
2. Formatter Registration (lua/reform/formatters.lua)
✅ Good:

Clean registration of Stylua for Lua files
Maintains lazy loading pattern
No breaking changes to existing API
⚠️ Issue:

Inconsistent formatter mapping: Lines 18-22 still map Java, JavaScript, JSON, ActionScript, and Ruby to Python formatter. This seems like a legacy mapping that might need review.
3. Configuration Updates (lua/reform/config.lua)
✅ Good:

Clean addition of Stylua to default formatters
Maintains existing configuration structure
4. Documentation (README.md)
✅ Good:

Clear requirements section updated with Stylua
Configuration examples updated
Maintains consistency with existing docs
Security Analysis
Low risk: Code appears non-malicious
Minor concern: Shell command execution via vim.fn.system() could be hardened
No secrets or credentials exposed
Performance Considerations
Good: Caching in is_available() prevents repeated PATH lookups
Good: Lazy loading maintains <1ms startup time
Potential: Large Lua files might block on system calls (no async formatting)
Test Coverage
Missing: No test files found for the new formatter
Recommendation: Add tests for:
Stylua availability detection
Formatting behavior with various Lua code samples
Error handling when Stylua fails
Caching behavior verification
Specific Recommendations
High Priority:
Fix security issue in command execution:
-- Safer implementation
local cmd = {"stylua", "-"}
local result = vim.fn.system(cmd, text)
Improve error handling:
function StyluaFormatter:format(text, filetype)
if text == "" then
return text
end

local cmd = {"stylua", "-"}
local result = vim.fn.system(cmd, text)

if vim.v.shell_error ~= 0 then
vim.notify("Reform: Stylua formatting failed", vim.log.levels.WARN)
return text
end

return vim.trim(result)
end
Add Stylua configuration support:
function StyluaFormatter:format(text, filetype)
if text == "" then
return text
end

-- Look for .stylua.toml in project root
local config_path = vim.fn.findfile(".stylua.toml", ".;")
local cmd = config_path ~= "" and {"stylua", "--config-path", config_path, "-"}
or {"stylua", "-"}

local result = vim.fn.system(cmd, text)
-- ... rest of implementation
end
Medium Priority:
Add comprehensive tests for the formatter
Document Stylua-specific configuration options
Consider async formatting for large files
Low Priority:
Review legacy formatter mappings for non-Lua languages
Add debug logging for troubleshooting
Overall Assessment
The implementation is solid and follows established patterns. The security concern is minor but should be addressed. The formatter will work correctly as-is, but the suggested improvements would make it more robust and user-friendly.

Approval Recommendation: ✅ Approve with minor fixes - The security issue should be addressed before merging, but this is a good addition to the codebase.

Great! @claude Help me improve the code based on this review.

Copy link
Contributor

claude bot commented Jul 20, 2025

Claude finished @GowayLee's task —— View job


I'll analyze this and get back to you.

- Fix security vulnerability by using table-based command instead of string
- Add automatic .stylua.toml and stylua.toml configuration detection
- Improve error handling with user notifications on formatting failures
- Update documentation with Stylua configuration examples
- Maintain backward compatibility with existing functionality

Security fixes:
- Replace vim.fn.system(cmd, text) with vim.fn.system({stylua, -}, text)
- Add proper argument escaping through table-based command construction

Configuration improvements:
- Auto-detect .stylua.toml and stylua.toml in project hierarchy
- Support both dotted and non-dotted configuration filenames
- Add comprehensive documentation and examples

Co-authored-by: Li Haoyuan <[email protected]>
- Add caching mechanism for Stylua configuration file paths
- Cache config paths per working directory to avoid repeated lookups
- Improve performance by reducing file system operations
- Maintain compatibility with existing API and behavior

Co-authored-by: Li Haoyuan <[email protected]>
Copy link
Owner Author

@GowayLee GowayLee left a comment

Choose a reason for hiding this comment

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

Good job

@GowayLee GowayLee added the enhancement New feature or request label Jul 20, 2025
@GowayLee GowayLee merged commit cdbd646 into main Jul 20, 2025
1 of 2 checks passed
@GowayLee GowayLee deleted the claude/issue-4-20250719-1912 branch July 20, 2025 03:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Stylua Support
1 participant