fix: snippet tabbing, duplicates, and accuracy#104
Conversation
- Merged duplicate snippets into the same, having multiple prefixes. - Fixed '<%-' and '<%=' snippets having confusing descriptions - Spaces -> tabs for code-block creating snippets (ie 'if') - Added closing '}' to else statement, as you would never not need it. - Added snippet choice insertion feature to choose 'let' or 'const' in for..of loop.
There was a problem hiding this comment.
Pull request overview
This PR consolidates and improves EJS snippets by removing duplicates, correcting descriptions, improving formatting, and adding new functionality. The changes address issues #105 and #87 related to snippet quality and usability.
Key Changes:
- Consolidated duplicate snippets by combining them with multiple prefixes (e.g., both "ejs" and "<%" trigger the same snippet)
- Changed final tab stops from
$2to$0for better cursor positioning after snippet expansion - Replaced spaces with tabs for indentation in code-block snippets and added missing closing braces
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| snippets/snippets.json | Merged duplicate snippets, fixed descriptions for output tags, standardized indentation to tabs, added for...of loop with const/let choice, and added closing brace to else statement |
| README.md | Updated snippet documentation table to reflect new prefixes, clearer descriptions, and the new for...of loop snippet |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| "ejsinc", | ||
| "<%" |
There was a problem hiding this comment.
The prefix "<%" for the EJS Include snippet conflicts with multiple other snippets that also use "<%" as a prefix (EJS No Output, EJS For Loop, EJS For...Of Loop, EJS ForEach, EJS If Statement, EJS Else If Statement, and EJS Else Statement). This will cause ambiguity when users type <% and trigger the autocomplete. Consider using a more specific prefix like "ejsinclude" or removing this generic prefix from snippets where the specific prefix (like "ejsinc") is more appropriate.
| "ejsinc", | |
| "<%" | |
| "ejsinc" |
| "<% } else { %>", | ||
| " $1" | ||
| "<% } else if (${1:condition}) { %>", | ||
| "\t$0" |
There was a problem hiding this comment.
The else if snippet is missing a closing <% } %> tag, unlike the other conditional snippets. This creates an inconsistency: the else statement (lines 128-130) has the closing brace, but else if does not. An else if block should also close its opening {, similar to how the if statement does.
| "\t$0" | |
| "\t$0", | |
| "<% } %>" |
| | ---------- | --------- | -------------------------------------------------------- | | ||
| | `ejs→` | `<%` | `<% %>` – No output tag | | ||
| | `ejsesc→` | `<%=` | `<%= %>` – Output **escaped** value | | ||
| | `ejsraw→` | `<%-` | `<%- %>` – Output **raw/unescaped** value (injects HTML) | |
There was a problem hiding this comment.
The snippet prefix listed here is ejsraw→, but there's also ejsunesc→ as a prefix in the actual snippet definition (snippets.json lines 24-25). The README should document both prefixes or clarify which one is the primary one. Currently, ejsunesc→ is missing from the documentation.
| | `ejsraw→` | `<%-` | `<%- %>` – Output **raw/unescaped** value (injects HTML) | | |
| | `ejsraw→` | `<%-` | `<%- %>` – Output **raw/unescaped** value (injects HTML) | | |
| | `ejsunesc→`| `<%-` | `<%- %>` – Output **raw/unescaped** value (injects HTML) | |
Fixes #105 and #87
Changes
Because I completely redid the file, I merged a couple related PR's before I made my changes: PR 58 & 100