fix(cli): handle directories with no matching templates#2144
fix(cli): handle directories with no matching templates#2144cyphercodes wants to merge 1 commit intohandlebars-lang:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes the Handlebars CLI behavior when a directory argument yields zero matching templates (empty dir or extension filter mismatch), ensuring the CLI doesn’t show usage help incorrectly and instead proceeds with directory processing while warning the user.
Changes:
- Update CLI help-display condition to treat “directory provided but no templates found” as a valid invocation.
- Add a warning from the precompiler CLI path when directories were scanned but no templates matched.
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
bin/handlebars.mjs |
Adjusts help condition to account for opts.hasDirectory so directories don’t trigger help output when empty/filtered. |
lib/precompiler.js |
Emits a warning when directory scanning produces zero templates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| if (!opts.templates.length && opts.hasDirectory) { | ||
| console.warn('Warning: No templates found in the specified directory.'); |
There was a problem hiding this comment.
New warning output is not covered by existing unit tests and will currently leak to the real stderr because the test harness in spec/precompiler.js mocks console.log/console.error but not console.warn. Consider either (a) adding/adjusting tests to stub and assert console.warn for this case, or (b) routing this message through the existing console.error path so tests and consumers can consistently capture it.
| console.warn('Warning: No templates found in the specified directory.'); | |
| console.error('Warning: No templates found in the specified directory.'); |
| } | ||
|
|
||
| if (!opts.templates.length && opts.hasDirectory) { | ||
| console.warn('Warning: No templates found in the specified directory.'); |
There was a problem hiding this comment.
Warning message is fairly generic; it would be more actionable if it included the directory path(s) that were scanned and (when applicable) the extension filter being used (e.g., --extension). This helps users distinguish between an empty directory vs. a directory with files that were filtered out.
| console.warn('Warning: No templates found in the specified directory.'); | |
| const dirs = | |
| (Array.isArray(opts.directories) && opts.directories.length | |
| ? opts.directories.join(', ') | |
| : opts.directory) || 'unknown'; | |
| const extInfo = opts.extension ? ` with extension filter "${opts.extension}"` : ''; | |
| console.warn( | |
| `Warning: No templates found in the specified directory/directories (${dirs})${extInfo}.` | |
| ); |
| } | ||
|
|
||
| if (opts.help || (!opts.templates.length && !opts.version)) { | ||
| if (opts.help || (!opts.templates.length && !opts.hasDirectory && !opts.version)) { |
There was a problem hiding this comment.
This line is ~86 chars and is what's breaking CI — npm run lint:format (oxfmt --check) fails on bin/handlebars.mjs in the latest run. Running npm run lint:format -- --fix (or the oxfmt equivalent) will wrap it and unblock the build.
| } | ||
|
|
||
| if (!opts.templates.length && opts.hasDirectory) { | ||
| console.warn('Warning: No templates found in the specified directory.'); |
There was a problem hiding this comment.
Separately from the other feedback on this line: once we've warned the user that nothing was found, the function still falls through and executes the whole output pipeline below. With opts.output set, that means we silently overwrite the user's output file with the empty scaffold (function() { var template = Handlebars.template, templates = {}; })(); while also telling them no templates were found — which is surprising and can clobber a previously-good artifact.
Consider return; right after the warning (or turning this into a thrown Handlebars.Exception so callers see a non-zero exit), so we don't produce output when we just reported there's nothing to produce.
|
A couple of higher-level notes that don't fit on a specific line: Test coverage for the new behavior is missing. The PR description says "All existing unit tests pass," but no new test was added. The pre-existing Worth grounding the warning text in #2109's actual root cause. Re-reading the issue thread, the problem was almost certainly that their templates used |
dbd30de to
4cf387d
Compare
|
Updated this PR to address the recent review feedback:
Local verification:
|
Fixes #2109
Previously, when passing a directory to the handlebars CLI that either had no template files or no files matching the extension, the CLI would show the help message instead of processing the directory.
This fix:
The CLI now properly processes directories and provides feedback when no templates are found instead of confusingly showing usage help.
Changes
bin/handlebars.mjs: Added!opts.hasDirectoryto the help conditionlib/precompiler.js: Added warning when directory has no matching templatesTesting