Add check method to Prettier Node API. #1104
Conversation
index.js
Outdated
| @@ -95,6 +95,9 @@ module.exports = { | |||
| format: function(text, opts) { | |||
| return formatWithShebang(text, normalizeOptions(opts)); | |||
| }, | |||
| check: function(text, opts) { | |||
| return this.format(text, opts) === text; | |||
There was a problem hiding this comment.
It seems weird that if it format throws (syntax error on the input), the check function would throw instead of returning false.
bin/prettier.js
Outdated
| } catch (e) { | ||
| // Add newline to split errors from filename line. | ||
| process.stdout.write("\n"); | ||
| handleError(filename, e); |
There was a problem hiding this comment.
we actually don't want to print the error here, we just want to print all the filenames that are different. It's up to the user to actually run prettier on all those files to see what's going on.
There was a problem hiding this comment.
Actually, looks like the existing implementation does it incorrectly, good thing you are looking into it :)
|
Oh man, crazy that you are looking into adopting it at Netflix! That seems like a good idea to me. Could you fix my comments around error handling and this should be good to go. Given that the function can throw and you need to handle it more carefully, I think it makes sense to have a check function for it instead of pushing this unexpected complexity to the call sites. |
|
@vjeux Great suggestions in your comments; I fixed your feedback. Did you want me to fix how |
bin/prettier.js
Outdated
| process.exitCode = 1; | ||
| } | ||
| } catch (e) { | ||
| // should never hit this block since check catches errors before it returns |
There was a problem hiding this comment.
if it never reaches this, can you just remove the entire try catch? :)
There was a problem hiding this comment.
Ha, fair. I'm just used to providing for IE8. There's a pretty slim chance that this will be run in IE8. :D
bin/prettier.js
Outdated
| @@ -232,6 +232,19 @@ if (stdin) { | |||
|
|
|||
| let output; | |||
|
|
|||
| if (argv["list-different"]) { | |||
There was a problem hiding this comment.
I would put this just above const start = Date.now(); since we don't really need those two variables to do that block of code.
|
|
bin/prettier.js
Outdated
| @@ -228,6 +228,16 @@ if (stdin) { | |||
| return; | |||
| } | |||
|
|
|||
| if (argv["list-different"]) { | |||
| try { | |||
There was a problem hiding this comment.
I don't think just a try parses!?
There was a problem hiding this comment.
Hmm, did you forget to push your commit? This seems to be the last one on master. If you remove that try, I'll merge it!
|
Yay, thanks! |
|
I'll do a release sometimes this week. Now that a lot of people are using prettier, need to polish them a bit more :p |
|
Works for me! I'll refactor the Gulp plugin and release it shortly thereafter. The maintainer of |
lock
bot
added
the
locked-due-to-inactivity
Hi friends!
I figured this was better as a PR instead of an issue. We'd like to adopt Prettier and part of that is build it into our continuous integration. We run everything through Gulp (hence why I also forked btholt/gulp-prettier too) which makes use of the Node API. The
list-differentwhich is available to the CLI is precisely what we need but since that lives in the bin and not in the Node library it can't be used. This moves this logic back into the library exported fromindex.jsas a tiny function that just makes use of theformatfunction.Is it very hard to keep ahold of the old string and compare it with the new string from Prettier? No, definitely not. This is what I did with my fork of gulp-prettier (which I borrowed the idea from facebook/react#9187). But I figured if my use case needed this as well as there's that it may be useful to support this use case in Prettier.
Let me know what you think and/or what I can fix!