Conversation
There was a problem hiding this comment.
Code Review: README.md Changes
Summary
This PR adds an Emacs lsp-mode configuration example for the emmylua_ls language server. The documentation is generally well-structured and follows the existing <details> pattern used elsewhere in the README.
Issues Found
1. Trailing Whitespace / Tab Character
Line after <summary><b>Emacs</b></summary>:
lsp-mode example:
There is a line containing only a tab character (\t) between the summary and the code block. This is a whitespace-only line that will likely trigger linter warnings and is inconsistent with the rest of the file.
Recommendation: Remove the tab and use a single blank line (or no blank line) for consistency.
2. Extra Blank Line Before Next <details>
+</details>
+
+
<details>
<summary><b>Other editors</b></summary>
Two blank lines were added before the "Other editors" section, while other sections in the file appear to use a single blank line separator.
Recommendation: Reduce to a single blank line to match the surrounding style.
3. Missing Language Identifier on Code Fence (Minor)
The code block uses ```elisp, which is correct. However, verify consistency with other editor examples in the README — if they use ```lua, ```json, etc., this is fine. No action needed if consistent.
4. Potential Functional Concern — cargo install Assumption
"cargo" "install" "emmylua_ls"The download function assumes emmylua_ls is installable via cargo install emmylua_ls. Please verify:
- The actual crate name on crates.io (it may differ from the binary name, e.g.,
emmylua_lsvsemmylua-ls). - Whether
cargois guaranteed to be on the user'sPATH.
If the crate name is incorrect, users following this doc will get a build failure. Consider adding a brief note or link to installation instructions.
5. :priority 0 May Be Intentional — Worth a Comment
:priority 0A priority of 0 is lower than the default lsp-mode clients. If this is intentional (to avoid overriding other Lua servers), it's fine, but a short comment explaining the choice would help users understand.
6. lsp-activate-on "lua" vs lua-mode
The activation function uses "lua" (language ID), while the hook uses lua-mode. These are consistent with lsp-mode conventions, so no change needed — just flagging for reviewer awareness.
Recommendations
- Required: Remove the tab-only line and the extra blank line.
- Suggested: Verify the
cargo installcrate name and add a note if installation differs from the binary name. - Optional: Add a brief comment about
:priority 0if it's a deliberate choice.
Verdict
Minor formatting cleanup needed before merge; the functional content looks reasonable assuming the cargo install target is correct.
Add emacs config example, via lsp-mode