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

Use prettier for code formatting #4090

Merged
merged 8 commits into from Jul 2, 2018
Merged

Use prettier for code formatting #4090

merged 8 commits into from Jul 2, 2018

Conversation

@gnestor
Copy link
Contributor

@gnestor gnestor commented Mar 2, 2018

Closes #3494

I’ve been using prettier for other projects for a while now and it’s been really nice because I don’t need to think about code style anymore.

I opted for the second option for adding prettier to an eslint/tslint project. This will replace our tslint rules with prettier’s rules so that there are no conflicts.

This PR also removes eslint since AFAIK this project uses tslint. Let me know if there is a good reason to keep it.

Lastly, I added a pre-commit hook to our git hooks that will format any staged typescript files before committing them. This should help maintain code style consistency whether contributors are aware of it or not.

To do:

  • Update Typescript Style Guide
  • Update travis_script.sh to use prettier vs. eslint?
    • Use eslint --fix to automatically fix formatting errors
@gnestor gnestor changed the title Use prettier Use prettier for code formatting Mar 2, 2018
@blink1073
Copy link
Member

@blink1073 blink1073 commented Mar 2, 2018

We we using eslint for our handful of .js files.

@rgbkrk
Copy link
Member

@rgbkrk rgbkrk commented Mar 2, 2018

I wholeheartedly approve!

Mostly so I can just keep prettier turned on when working in other projects. 😄

@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Mar 2, 2018

Ok, I added back eslint and added prettier support for JS, CSS, JSON, and MD. Hopefully this will fix the build issue 🙏

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Mar 2, 2018

Leaving this as a reminder to myself, or whoever wants to tackle it: we should update the style guide.

@gnestor gnestor force-pushed the prettier branch 2 times, most recently from a348f68 to 4f4b512 Mar 2, 2018
@blink1073
Copy link
Member

@blink1073 blink1073 commented Mar 2, 2018

This is doing some odd things to markdown files like reordering things, I don't think we should use it there.

@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Mar 2, 2018

It definitely shouldn't be! From what I've read, it only does style/formatting and doesn't modify the AST. Where are you seeing that?

@blink1073
Copy link
Member

@blink1073 blink1073 commented Mar 3, 2018

My mistake, I misread the diff. The PR has a conflict after #4098.

.eslintignore Outdated
@@ -4,3 +4,6 @@ node_modules
**/build
jupyterlab/staging/yarn.js
jupyterlab/static

dev_mode/index.js
jupyterlab/staging/index.js
Copy link
Contributor Author

@gnestor gnestor Mar 9, 2018

I had to disable eslint on these 2 files because simply adding them to .prettierignore didn't ignore them from eslint.

@@ -49,11 +50,15 @@
},
"dependencies": {},
"devDependencies": {
"@jupyterlab/eslint-plugin-jinja": "^0.2.0",
Copy link
Contributor Author

@gnestor gnestor Mar 9, 2018

I removed @jupyterlab/eslint-plugin-jinja dependency because it was only used for the 2 files that are now ignored by eslint

@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Mar 9, 2018

How do we want to enforce prettier formatting for contributions in general? If a contributor is aware of prettier in this repo, then he/she can use an editor extension to run it themselves or install the git pre-commit hook. We can't count on that though (especially since there is currently no documentation about prettier in this repo). Can we run a git hook in Travis that function like the pre-commit hook (gets all commits in a PR, for each commit get files that prettier can process, run prettier on them, and git add them back to the commits)?

@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Mar 9, 2018

I also see this warning a lot in jupyterlab and extension repos:

Value is not accepted. Valid values: "es5", "es6", "es2015", "es7", "es2016", "es2017", "es2018", "esnext", "dom", "dom.iterable", "webworker", "scripthost", "es2015.core", "es2015.collection", "es2015.generator", "es2015.iterable", "es2015.promise", "es2015.proxy", "es2015.reflect", "es2015.symbol", "es2015.symbol.wellknown", "es2016.array.include", "es2017.object", "es2017.sharedmemory", "es2017.string", "es2017.typedarrays", "es2018.promise", "es2018.regexp", "esnext.array", "esnext.asynciterable".

Can we make values in the lib property of tsconfig.json files lowercase moving forward?

@blink1073
Copy link
Member

@blink1073 blink1073 commented Mar 9, 2018

The way we handle integrity is by running on CI and if it fails prompting to run integrity locally. I don't think we can have Travis "inject" a commit. This could be handled similarly. Sure, lower casing those makes sense.

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Mar 14, 2018

While I was looking at the prettier-standard project, I saw these two tools, which might be useful: https://github.com/okonet/lint-staged https://github.com/typicode/husky. I think they could be combined instead of the custom pre commit hook you added, with the same effect.

@rgbkrk
Copy link
Member

@rgbkrk rgbkrk commented Mar 14, 2018

That's exactly what we use in nteract, lint-staged and husky --

Though now that I look at it, I'm not sure what husky is doing since it looks like lint-staged does all the work.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Mar 15, 2018

In the JLab dev meeting, we decided to not merge this until we've merged a bunch of PRs and released beta 2, so setting to the beta 3 milestone. That also gives us a little time to check out the formatting results.

@jasongrout jasongrout added this to the Beta 3 milestone Mar 15, 2018
@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Mar 16, 2018

Thanks @saulshanabrook and @rgbkrk! I've looked at the options at https://prettier.io/docs/en/precommit.html. It looks like precise-commits has an advantage over lint-staged when it comes to partial file formatting. I've already implemented the bash script method, so the missing part is forcing users to use the pre-commit hook. I don't know what husky does exactly, but if we had an npm postInstall script that would install the git hook, then everyone should be formatting their files before committing and publishing.

@jasongrout jasongrout removed this from the Beta 3 milestone Apr 18, 2018
@jasongrout jasongrout added this to the Beta 2 Patch milestone Apr 18, 2018
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 23, 2018

@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Jun 23, 2018

@jasongrout All good!

@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Jul 2, 2018

@ian-r-rose @afshin Ok, we're finally rebased and passing. Merge at will 👍

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Jul 2, 2018

Thanks for your patience @gnestor! Merging before anything else conflicts...

@ian-r-rose ian-r-rose merged commit 1aff419 into jupyterlab:master Jul 2, 2018
2 checks passed
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jul 2, 2018

I had some issues trying to rebase a PR after these changes. This is what worked: #4330 (comment)

@bollwyvl
Copy link
Contributor

@bollwyvl bollwyvl commented Jul 3, 2018

@gnestor gnestor deleted the prettier branch Jul 3, 2018
@ian-r-rose ian-r-rose mentioned this pull request Jul 20, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

9 participants