Skip to content
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

Lint code with eslint and format with prettier #1172

Merged
merged 1 commit into from
Sep 28, 2018

Conversation

kevgathuku
Copy link
Contributor

@kevgathuku kevgathuku commented Sep 27, 2018

Resolves #1130

Setup largely based off reading, which also contains info on how to configure your editor
https://medium.com/@doppelmutzi/eslint-prettier-vue-workflow-46a3cf54332f

To show errors on console, run:
npm run lint

To auto-fix errors, run:
npm run lint:autofix

  • This causes a lot of changes so I've decided to hold off on actually running the auto-formatter to avoid conflicts.

The prettier config is in package.json

@skjnldsv
Copy link
Member

@kevgathuku I'm not familiar that much with prettier, but what does it do more than eslint in the end? :)

@kevgathuku
Copy link
Contributor Author

kevgathuku commented Sep 27, 2018

@skjnldsv it's mostly for consistent code formatting
e.g. single/double quotes, spaces/tabs, etc.

When using eslint + prettier together, eslint does the code checking e.g. unused variables and prettier does the formatting

Does this make sense?

@skjnldsv
Copy link
Member

@kevgathuku it does :)
But eslint is already doing this on our setup on contacts for example.
https://github.com/nextcloud/contacts/blob/vue/.eslintrc.js

It's already doing the spaces/tabs checks, quotes, alignments, it even handle the coding style like spaces between curly, maximum number of functions (.then, .catch ...) per lines, stuff like that 🤔

@kevgathuku
Copy link
Contributor Author

kevgathuku commented Sep 27, 2018

@skjnldsv from what I can tell from looking at the config, you are using
standard instead of prettier for the formatting

https://github.com/nextcloud/contacts/blob/vue/.eslintrc.js#L25

@skjnldsv
Copy link
Member

@kevgathuku aAaah, awesome! I thought it was the standard default for eslint (I don't really know that much on eslint and stuff :) )

That explains a lot!! Maybe we should move to prettier too then? :)
prettier/prettier-eslint#101

@kevgathuku
Copy link
Contributor Author

@skjnldsv I haven't used standard before so not sure about the move
How's your experience using it on the contacts app?
It might actually be better for us to move to it if you endorse it :)

Watching https://www.youtube.com/watch?v=kuHfMw8j4xk to educate myself about the differences between prettier and standard in the meantime 😄

@skjnldsv
Copy link
Member

@kevgathuku Well, I'll be honest, I'm very fond of the current coding style we have on contacts. Working with two beginners (which are not really beginners anymore, but still discovering various coding styles), they seemed to be very happy on how it is displayed in the end and very clear.

I tried quickly to setup the default styles of prettier, and It feels far too spaced for me. I know that some people like having new lines everywhere, but I don't really. Take a look at the code we have and tell me if you think it looks clear enough?

@kevgathuku
Copy link
Contributor Author

I'll check it out @skjnldsv 👍

@kevgathuku
Copy link
Contributor Author

kevgathuku commented Sep 27, 2018

I've checked through the contacts app code @skjnldsv and it looks pretty neat 👍

I was trying to go for the same setup here i.e. using tabs and 4 spaces per tab.
https://github.com/nextcloud/mail/pull/1172/files#diff-b9cfc7f2cdf78a7f4b91a753d10865a2R73

Once formatted, they would end up looking the same, just using different formatters 😄

screen shot 2018-09-27 at 21 52 12

Left: Contacts app
Right: Mail app with prettier setup

To conclude, I'm not particularly tied to any tool, whatever we agree works best is what we'll go with

@ChristophWurst
Copy link
Member

Conflicting files

package.json

Also, there are commits from #1171 here, so you might want to merge lastest master to update this branch.

I have no preference on tools as long as they get the job done. I assume it's more important to choose consistent rules than exact tools. If the rules for styling are (almost) the same, it should also be easy to switch to a different linter later on.

@skjnldsv
Copy link
Member

I have no preference on tools as long as they get the job done. I assume it's more important to choose consistent rules than exact tools. If the rules for styling are (almost) the same, it should also be easy to switch to a different linter later on.

Exactly! 👍
Btw @kevgathuku don't forget the recommended vue rule ;)

@kevgathuku
Copy link
Contributor Author

Thanks for the input @skjnldsv @ChristophWurst
I've fixed the conflicts and added the recommended vue rule.

@ChristophWurst
Copy link
Member

@kevgathuku is this ready for review? Please set the appropriate labels (2 developing or 3 to review) 😉

@kevgathuku
Copy link
Contributor Author

@ChristophWurst yep it's ready.
But I haven't run the actual formatting on the files. I propose we do that separately since it will contain lots of changes

@ChristophWurst
Copy link
Member

I propose we do that separately since it will contain lots of changes

Sure. Let's maybe do that in chunks of a few components each or maybe per folder in the js dir.

@ChristophWurst ChristophWurst added this to Needs review in Vue Rewrite via automation Sep 28, 2018
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@ChristophWurst ChristophWurst merged commit 92a433a into refactor/vue Sep 28, 2018
Vue Rewrite automation moved this from Needs review to Done Sep 28, 2018
@ChristophWurst ChristophWurst deleted the feature/prettier branch September 28, 2018 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Vue Rewrite
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants