New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: implement #1

Merged
merged 13 commits into from Jan 19, 2017

Conversation

Projects
None yet
3 participants
@kentcdodds
Copy link
Member

kentcdodds commented Jan 15, 2017

Still working on this.

kentcdodds added some commits Jan 14, 2017

@kentcdodds

This comment has been minimized.

Copy link
Member

kentcdodds commented Jan 18, 2017

Ping @aharris88, could you try this out?

npm install prettier-eslint-cli@beta
./node_modules/.bin/prettier-eslint --help

Try it out on a project. I think that it's a solid start.

@kentcdodds

This comment has been minimized.

Copy link
Member

kentcdodds commented Jan 18, 2017

Thinking it may be a good idea to add a --write option to actually override the files, otherwise it just prints out to stdout. This is what prettier's CLI does. It's safer.

@edm00se

This comment has been minimized.

Copy link

edm00se commented Jan 18, 2017

Trying the above, I'm getting a replacement of my processed JS file contents with "undefined".

macOS Sierra (10.12.2)
shell: zsh, w/ oh-my-zsh
Node: 6.9.1
npm: 3.10.9
(I normally run LTS latest, but apparently I'm behind a little)

Steps to reproduce:

  • git clone https://github.com/edm00se/personal-mock-url-shortener.git tmp
  • cd tmp
  • npm install
  • npm install prettier-eslint-cli@beta -D
  • ./node_modules/.bin/prettier-eslint server.js

The original source of the server.js file can be found here.

Note: this will look slightly different from my tweeted gif as my initial test was against a local, un push-ed feature branch (I grabbed whatever node app was open in a terminal, changes were staged, it's all good 😉). I cloned a local copy from scratch as outlined in the steps above, got the same results ("undefined" replacing my file contents). I also tried it against a globbed **/*.js, same deal for each js file.

@agarrharr

This comment has been minimized.

Copy link
Collaborator

agarrharr commented Jan 18, 2017

Looks like it's working for me. 👍

@agarrharr

This comment has been minimized.

Copy link
Collaborator

agarrharr commented Jan 18, 2017

Although, I can't seem to get it to pick up my eslint config.

@kentcdodds

This comment has been minimized.

Copy link
Member

kentcdodds commented Jan 18, 2017

That's good feedback @aharris88, could you dig into it a little bit?

@agarrharr

This comment has been minimized.

Copy link
Collaborator

agarrharr commented Jan 18, 2017

And this is what I have in my .vimrc

autocmd FileType javascript set formatprg=./node_modules/.bin/prettier-eslint\ --stdin
@agarrharr

This comment has been minimized.

Copy link
Collaborator

agarrharr commented Jan 18, 2017

Ok, I guess I had to reload vim. I'm now getting my file replaced with this:

success formatting 0 files with prettier-eslint ✨
@agarrharr

This comment has been minimized.

Copy link
Collaborator

agarrharr commented Jan 18, 2017

It works if I run it from the command line (and it's reading my .eslintrc file), but for some reason, when I run it in vim, it's not getting the file as input.

@kentcdodds

This comment has been minimized.

Copy link
Member

kentcdodds commented Jan 18, 2017

--stdin isn't implemented (yet... I'm working on it now actually 😄)

@kentcdodds

This comment has been minimized.

Copy link
Member

kentcdodds commented Jan 18, 2017

@edm00se, I believe that this should fix your issue. Could you clear your npm/yarn cache and try again?

@agarrharr

This comment has been minimized.

Copy link
Collaborator

agarrharr commented Jan 18, 2017

I actually just got the issue where it output undefined, and when I reinstalled, it fixed the issue.

@edm00se

This comment has been minimized.

Copy link

edm00se commented Jan 18, 2017

@kentcdodds This certainly fixes my updating to "undefined" in my files. I'm not sure that it's adhering to my eslint config, either in my package.json (initial) or in a (newly created) .eslintrc with the same contents.

Padding before function parens are disappearing, etc. (opened in Atom just for visual comparison, file wasn't even open during ./node_modules/.bin/prettier-eslint server.js run)
eslint_formatting

@kentcdodds

This comment has been minimized.

Copy link
Member

kentcdodds commented Jan 18, 2017

Yeah, that's probably prettier doing that :) Try pasting your code in here and see what happens (note that the prettier config is generated via your eslint config, so you may need to adjust some things). But I think it's working!

@edm00se

This comment has been minimized.

Copy link

edm00se commented Jan 18, 2017

The output from pasting into that link looks pretty consistent, with the exception of that trailing comma (which is what's causing Atom to complain, since it only seems to understand my eslint config).

@agarrharr

This comment has been minimized.

Copy link
Collaborator

agarrharr commented Jan 18, 2017

print-width is the option in prettier that is probably causing that multi-line issue. It defaults to 80. How do you set it with prettier-eslint-cli?

@kentcdodds

This comment has been minimized.

Copy link
Member

kentcdodds commented Jan 18, 2017

The CLI doesn't expose any mechanism for customizing prettier currently (I don't think it'll be necessary). Instead, prettier-eslint will generate the prettier options based on the eslint config for the file it's formatting. The relevant eslint rule is max-len

@kentcdodds

This comment has been minimized.

Copy link
Member

kentcdodds commented Jan 18, 2017

@edm00se, I'm not certain about the trailing comma. 🤔 We should probably expose a way for you to log out the eslintConfig and prettierOptions that are used for the formatting. Would you be interested in doing that? Would probably just need another option (like sillyLogs) and add the console.log here that includes the eslintConfig and prettierOptions (maybe put it in a separate function and just call it there... Wanna try?

@edm00se

This comment has been minimized.

Copy link

edm00se commented Jan 18, 2017

I'm game, but it'll probably have to be later in my day :'-( I can give it a try tonight at least.

@kentcdodds

This comment has been minimized.

Copy link
Member

kentcdodds commented Jan 18, 2017

I'll file an issue you can use to reference.

@kentcdodds kentcdodds referenced this pull request Jan 18, 2017

Closed

Add sillyLogs #7

@agarrharr

This comment has been minimized.

Copy link
Collaborator

agarrharr commented Jan 18, 2017

If you have invalid syntax, you get this error:

(node:98856) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): SyntaxError: Unexpected token, expected ) (202:9)
(node:98856) DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
@kentcdodds

This comment has been minimized.

Copy link
Member

kentcdodds commented Jan 18, 2017

Ah! Great point @aharris88! Thanks for that! I forgot to handle errors :)

@kentcdodds

This comment has been minimized.

Copy link
Member

kentcdodds commented Jan 18, 2017

Alrighty! Could I get you guys to try npm cache clean prettier-eslint-cli && npm install prettier-eslint-cli@beta && ./node_modules/.bin/prettier-eslint --version

That should give you 1.0.0-beta.1. If you could make sure that works for your stuff that'd be great! (@aharris88, there's now a --stdin option, and we handle errors gracefully).

Thanks for all the help and feedback!

@edm00se

This comment has been minimized.

Copy link

edm00se commented Jan 18, 2017

Uh... both --help and --version fail for me. npm cache clean prettier-eslint-cli && npm install prettier-eslint-cli@beta worked fine.

screenshot 2017-01-18 15 31 43

@kentcdodds

This comment has been minimized.

Copy link
Member

kentcdodds commented Jan 18, 2017

Ran into a quick error and published a 1.0.0-beta.2 :)

@edm00se

This comment has been minimized.

Copy link

edm00se commented Jan 18, 2017

Runs --version clean now, reporting 1.0.0-beta.2.

@kentcdodds

This comment has been minimized.

Copy link
Member

kentcdodds commented Jan 18, 2017

Whoops! Forgot to add the --write flag. You have to specify that if you want it to update your files! Pushed out 1.0.0-beta.3 with that fix!

@kentcdodds

This comment has been minimized.

Copy link
Member

kentcdodds commented Jan 18, 2017

Things are looking pretty good from my perspective. I'm finding a bunch of issues with prettier I think :)

@kentcdodds kentcdodds merged this pull request into master Jan 19, 2017

1 check passed

dependency-ci Dependencies checked
Details

@kentcdodds kentcdodds deleted the pr/implement branch Jan 19, 2017

@kentcdodds

This comment has been minimized.

Copy link
Member

kentcdodds commented Jan 19, 2017

Will setup automatic releases tomorrow morning! Cheers!

@kentcdodds

This comment has been minimized.

Copy link
Member

kentcdodds commented Jan 19, 2017

Hey friends! This has been released as 1.0.0! (I managed to slip in --silly-logs as well)

@aharris88, could you add your vim usage example to the examples? Be sure to update your contributions in .all-contributorsrc to include example :)

Thanks a bunch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment