Add --fix-to-stdout #53
Conversation
| @@ -74,12 +74,19 @@ module.exports = function (cwd, args, text) { | |||
|
|
|||
| return JSON.stringify(fileConfig, null, ' '); | |||
| } | |||
| if (currentOptions.fixToStdout && !stdin) { | |||
There was a problem hiding this comment.
Should I check for text here? tbh, the check for text and the fallback to using files below is confusing. It seems stdin should always use stdin, even if the stdin is empty.
There was a problem hiding this comment.
Hm, I see. Checking for text shouldn't be necessary. I agree that the flag should be enough.
| var report; | ||
| if (stdin && text) { | ||
| report = engine.executeOnText(text, currentOptions.stdinFilename); | ||
| } else { | ||
| report = engine.executeOnFiles(files); | ||
| } | ||
| if (currentOptions.fixToStdout) { | ||
| return report.results[0].output || text; |
There was a problem hiding this comment.
Related to above, there's an edge case here if a user passes --stdin, no text, and more than one file. I should probably at least check for text in the conditional, but that would introduce another edge case... if a user lint fixed a blank file, you'd want it to return a blank file (maybe with a trailing newline? 😄)
There was a problem hiding this comment.
Maybe the new option should only allow a single file if not used with --stdin? This way we don't need to require the --stdin option and one could use the feature to "inspect" what eslint does with --fix without actually modifying the file.
There was a problem hiding this comment.
is there an easy way to detect a single file? does eslint_d expand globs/directories or does it let eslint do that
There was a problem hiding this comment.
Globs are expanded by eslint. Let's take the easy path then and only support --stdin then. If text is empty, we just do nothing.
| option: 'fix-to-stdout', | ||
| type: 'Boolean', | ||
| description: 'Print the fix results to stdout instead of regular output, ' | ||
| + 'must be used with --stdin' |
There was a problem hiding this comment.
Should we assume --stdin or just require it?
|
@mantoni I think this is ready for review |
With Emacs instructions
aaronjensen
force-pushed
the
add-fix-stdout
branch
from
af952f3
to
f2567d9
Compare
|
Looks good to me. Thanks a ton! 😎 Will merge and release later today or tomorrow … |
|
Released in |
|
Nice. Unfortunately, I think that supporting non-stdin would be even trickier than knowing the file count. If there are no changes, I believe that eslint reports no source so we'd need to actually read the source from the file and send it back. Otherwise we could just count the # of results that came back and error if there are more than one. |
|
I think it was a good idea to make it work with a simple code path first. We can iterate on it once we have a good feeling about how this could work. I'm currently not a 100% sure about the approach we should take here. Maybe it's also not too important to have. Let's see. |
| ```elisp | ||
| (defun eslint-fix () | ||
| (interactive) | ||
| (setq temp-point (point)) |
There was a problem hiding this comment.
@aaronjensen Why don't you let-bind this variable? It's not a good idea to just setq variables this way.
There was a problem hiding this comment.
Actually you could just use save-excursion here, if I'm not mistaken. That'll restore point automatically afterwards. Did you try it?
There was a problem hiding this comment.
Unfortunately, save-excursion doesn't work w/ shell-command-on-region, that was the first I tried. Good idea on let though.
fixes #52