feat: add support for --stdin with --fix option#554
feat: add support for --stdin with --fix option#554kaichen wants to merge 5 commits intoigorshubovych:masterfrom
Conversation
DavidAnson
left a comment
DavidAnson
left a comment
There was a problem hiding this comment.
Looks great, very thorough!! Just a few comments and questions in the first pass.
| .option('--configPointer <pointer>', 'JSON Pointer to object within configuration file', '') | ||
| .option('-d, --dot', 'include files/folders with a dot (for example `.github`)') | ||
| .option('-f, --fix', 'fix basic errors (does not work with STDIN)') | ||
| .option('-f, --fix', 'fix basic errors') |
There was a problem hiding this comment.
Thanks. Please update README.md with the same change.
markdownlint.js
Outdated
| const fixResult = lint(fixOptions); | ||
| const fixes = fixResult.stdin.filter(error => error.fixInfo); | ||
| let outputText = stdin; | ||
| if (fixes.length > 0) { |
There was a problem hiding this comment.
Unnecessary check? Should be fine to remove.
markdownlint.js
Outdated
|
|
||
| if (options.output) { | ||
| // Write content to output file | ||
| try { |
There was a problem hiding this comment.
Not easy to tell on my phone, but this feels duplicated? Is there a clean way to reuse? Maybe not, but worth looking.
markdownlint.js
Outdated
| console.warn('Cannot write to output file ' + options.output + ': ' + error.message); | ||
| process.exitCode = exitCodes.failedToWriteOutputFile; | ||
| } | ||
| } else if (!options.quiet) { |
There was a problem hiding this comment.
Is this a scenario anyone would use? stdin with quiet? I feel like maybe it should not care about quiet?
There was a problem hiding this comment.
You're right, stdin definitely conflicts with quiet, they can't be used together. It's just that the options earlier didn't check for that. Logically, this part can be removed.
markdownlint.js
Outdated
| process.stdout.write(outputText); | ||
| } | ||
|
|
||
| return; // Exit early when fixing stdin |
There was a problem hiding this comment.
As above, I don't love this pattern, but maybe the alternative is worse?
There was a problem hiding this comment.
If we don't do an early return here, then the code that handles the file below would have to be wrapped inside an else block.
test/test.js
Outdated
| const stdin = {string: ['## Not a first level heading', '', 'Text content'].join('\n')}; | ||
| const result = await spawn('../markdownlint.js', ['--stdin', '--fix', '--config', 'test-config.json'], {stdin}); | ||
| t.is(result.stdout, stdin.string); | ||
| t.is(result.stderr, ''); |
There was a problem hiding this comment.
Shouldn't stderr still contain the unfixable errors in this case?
If so, I'd ask for one more test that fixes some of the total errors and stderr outputs the unfixed ones.
DavidAnson
left a comment
DavidAnson
left a comment
There was a problem hiding this comment.
Thank you for the updates! Good catch adding the third parameter to writeFileSync. :)
There is one area of simplification and I think the meaning of the output argument may be wrong?
| if (stdin) { | ||
| // Handle fix for stdin | ||
| const fixResult = lint(fixOptions); | ||
| const fixes = fixResult.stdin.filter(error => error.fixInfo); |
There was a problem hiding this comment.
No need to filter here, I think - pass the full list and it will ignore what it cannot do.
| outputText = applyFixes(stdin, fixes); | ||
|
|
||
| if (options.output) { | ||
| writeOutputFile(options.output, outputText); |
There was a problem hiding this comment.
The README says this file contains the issues, not the file content - I do not think this should be used for both purposes?
| const checkOptions = {...lintOptions}; | ||
| checkOptions.strings = {stdin: outputText}; | ||
| const checkResult = lint(checkOptions); | ||
| if (Object.keys(checkResult).some(file => checkResult[file].length > 0)) { |
There was a problem hiding this comment.
There is only one input just like line 321, so it can be used directly here.
|
|
||
| // Check for remaining errors after fix | ||
| const remainingErrors = lint(lintOptions); | ||
| const hasErrors = Object.keys(remainingErrors).some(file => remainingErrors[file].length > 0); |
There was a problem hiding this comment.
Same comment as above, only one key will be present.
| const remainingErrors = lint(lintOptions); | ||
| const hasErrors = Object.keys(remainingErrors).some(file => remainingErrors[file].length > 0); | ||
|
|
||
| process.stdout.write(outputText); |
There was a problem hiding this comment.
Please move this line above the previous lines setting remainingErrors/hasErrors.
| t.is(result.exitCode, 0); | ||
| }); | ||
|
|
||
| test('--stdin --fix --output writes fixed content to file', async t => { |
There was a problem hiding this comment.
See above for why I think this is testing the wrong behavior.
2 participants
This PR enables the --stdin and --fix options to work together, allowing users to pipe markdown content through markdownlint-cli and receive fixed output. Previously, using these options together would show the help message. The implementation applies fixes to stdin content and outputs to stdout by default, with support for --output and --quiet options.