Use Prettier #1316

wmertens · 2020-04-23T22:01:40Z

It's very nice to not have to worry about formatting. Most of the time the result is very acceptable, and by formatting on save in editors that support it, you never have to spend time on getting the spacing right.

It even helps with spotting bugs, because the formatting will show nesting errors.

ylecuyer · 2020-04-23T22:28:39Z

If this hasn't changed recently, the project has already jshint taking care of this.

wmertens · 2020-04-23T23:20:46Z

Ah - I didn't notice.

In any case, as far as i know, prettier is the only formatter that formats the way it does, other formatters just follow some rigid rules, and prettier optimizes for legibility and clean diffs.

It also formats markdown, html etc

Hirse

I'm strongly in favor of using an automatic formatter, but this PR adds so many changes, it might we worth looking into prettier's pragma-mode.
That way, files can be formatted over time.

Additionally, to ensure code is formatted, consider adding prettier --check to the npm test script.

.prettierignore

.prettierrc

CHANGELOG.md

.prettierignore

wmertens · 2020-04-24T11:46:06Z

changes applied. I decided against the pragma mode because it means adding a header to all files. If there's a problem, we can override it on a single directory.

Should the jshint config change?

wmertens · 2020-04-24T11:53:49Z

oh, and do we want (err) => ... or err => ...? I've seen both styles.

Personally, I prefer tabs, no semicolumns, any chance of that happening? ;)

wmertens · 2020-04-24T12:27:22Z

Ah, I see that jshint errors in the tests.

What would be the sentiment around switching to eslint? I believe it's currently the more maintained one (just an impression) and it integrates with prettier.

wmertens · 2020-04-25T20:57:05Z

🤔 looks like prettier on Windows always fails, the other tests just time out sometimes.
Possibly line endings?

wmertens · 2020-04-25T21:52:04Z

Ok that was it. Now the only test failures are random timeouts

Hirse · 2020-04-26T02:37:15Z

package.json

@@ -6,8 +6,9 @@
  "ungitPluginApiVersion": "0.2.0",
  "scripts": {
    "start": "node ./bin/ungit",
-    "test": "grunt test",
+    "test": "prettier --check . && grunt test",


Currently all tooling is run through grunt and while I think it might be good to switch to npm scripts in general, that might be a different change.

my grunt is rusty, any quick hints for how to achieve this?

I would recommend looking at Gruntfile.js and grunt-prettier package

@jung-kim I ended up doing it in plain grunt, would the grunt-prettier package add anything?

.prettierrc

wmertens · 2020-04-27T15:22:10Z

rebased, moved test into grunt, reverted arrowParens to default

jung-kim · 2020-04-28T00:23:32Z

Should prettier replace existing js-hint?

I admit, I've never done js professional so I feel I'm always behind on "new standard" but it seems that prettier is supersedes js-hint which superseded js-lint.

Hirse · 2020-04-28T01:56:56Z

@jung-kim Not quite.
JSHint and JSLint do "linting", i.e. static analysis, finding of potential errors in the code (consider unused variables and parameters) as well as general best practices (consider issues like using var instead of let/const.)
Both are superseded by ESLint.

Prettier on the other hand is an automatic code formatter, focusing on code style (spaces around operators, etc).

Both (types of tools) can work well together when properly configured.

wmertens · 2020-04-28T13:08:34Z

As for linting: I'll gladly do an eslint one later, and in the nodegit branch I have the configuration to make VS Code use Intellisense on the JS files, which is great for detecting typing errors and method/argument completion.

campersau

Since this PR affects (conflicts) with existing PRs, we will go through some of the other ones first before merging this.

Gruntfile.js

.prettierrc

campersau · 2020-04-29T14:12:22Z

.gitattributes

@@ -1,3 +1,3 @@
-* text=auto
+* text=auto eol=lf


For reference: prettier/prettier#7853 (comment)

package.json

Gruntfile.js

wmertens · 2020-05-04T05:28:06Z

Changes applied. If you prefer, I can leave off the prettier commit so it will be easier to merge.

Another option (reasonably nuclear) is to rewrite the whole repo history with prettier. If the forks do the same then the PRs can remain as-is, but formatted. It takes quite a while to run though:

git filter-branch --tree-filter 'git show $GIT_COMMIT --pretty= --name-only --diff-filter=AM -z | \
    xargs -r -0 prettier --config '"$PWD"'/../prettierrc --ignore-path '"$PWD"'/../prettierignore --write || true' -- --all

campersau

Note: We have to run prettier twice the first time, because it does not format server.js and git-api.js correctly in the first run.

package.json

Gruntfile.js

wmertens · 2020-05-08T15:39:56Z

@campersau 👍 done

campersau

Thanks! Looks good now. I will wait until monday before merging to give others some time to comment since this will change a lot of code.

wmertens · 2020-05-08T16:12:01Z

Should I add a prettier commit so people can see the result?

campersau · 2020-05-08T16:17:48Z

Yes, you can. Please run it twice!

.prettierignore

Gruntfile.js

Hirse · 2020-05-08T18:24:38Z

Gruntfile.js

@@ -188,6 +177,21 @@ module.exports = (grunt) => {
    }
  });

+  grunt.registerTask('checkPrettier', 'Verify that all files are correctly formatted.', function() {
+    const done = this.async();
+    childProcess.exec('prettier -l . bin/*', (err, stdout, stderr) => {


I would prefer using the prettier check API instead of childProcess.

Also, if we are using a command anyway, I do not see much value by adding it to Grunt.

Yeah, I just tried to run grunt but it didn't work because I don't have prettier installed globally. Only npm run build works now.

While I agree that that is the cleaner way of going about it, it is also a lot more work and I just want to get code formatted :)

On top of that, it's not clear that Grunt will stay in use. Personally, I havent used Grunt since 2014, and instead I rely on Webpack and nps for my entire workflow. Grunt is just so much more complex.

And even if Grunt will stay in use, eslint is imho better than jshint, and it has prettier support for checking and fixing.

I added npx to the grunt command so that it will use the installed prettier.

Hirse · 2020-05-08T18:26:42Z

Gruntfile.js

+        return done(new Error(`Files with incorrect formatting (run "npm run format" and consider a Prettier plugin for your editor):\n${files}\n`))
+      }
+      if (err) {
+        console.error(stderr)


Use grunt.log instead of console.

Also, the line is missing a ;.

Suggested change

console.error(stderr)

grunt.log.error(stderr);

Hirse · 2020-05-08T18:36:26Z

After this PR (in its current state) I can see several different pattern that we might want to align:

Grunt plugins
less, watch, jshint
Custom Grunt tasks calling JS APIs
browserify (instead of grunt-browserify)
Custom Grunt tasks calling CLI APIs
prettier check
Calling CLI APIs from npm scripts
prettier format

What is the general idea for calling build steps/scripts?

Given the slowed down (or discontinued) maintenance of Grunt plugins, we might want to reduce their usage. Should we also migrate away from Grunt itself?

While this is not directly related to prettier, I wanted to mention it here, because this PR adds two more entries to my list above.

wmertens · 2020-05-09T07:30:14Z

@Hirse So I think webpack is more powerful than browserify. I have a setup where the dev server automatically updates the server (hot-patching the endpoints, reloading if not possible) and the client side (uses React to do the same), all from running a single command.

The prod build converts only the files needed for the final bundle to the formats needed, thanks to require-based dependencies and pluggable loaders.

This removes 90% of what grunt does, and does it as fast or faster. The other 10% are maintenance scripts that are annoying to write in Grunt; I instead run them with https://github.com/sezna/nps

Hirse · 2020-05-09T17:52:00Z

@wmertens I agree that webpack is powerful enough to handle most cases, but switching from browserify to webpack has lots of other consequences for the current plugin-infrastructure.

As a first step, replacing grunt with individual scripts (whether running them directly or through nps) would be good.

prettier refuses to format these

It should be the default but --check complains

* prettier: add and configure * fix html errors prettier refuses to format these * jslint: ignore prettier formatting * prettier: ensure lf line endings on windows It should be the default but --check complains * Move prettier check into grunt

Hirse reviewed Apr 24, 2020

View reviewed changes

.prettierignore Outdated Show resolved Hide resolved

.prettierrc Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

.prettierignore Outdated Show resolved Hide resolved

wmertens force-pushed the prettier branch from 15f1942 to c50ffb8 Compare April 24, 2020 11:43

wmertens force-pushed the prettier branch from 624ce7a to 1ec6109 Compare April 25, 2020 17:19

Hirse reviewed Apr 26, 2020

View reviewed changes

.prettierrc Show resolved Hide resolved

Hirse reviewed Apr 26, 2020

View reviewed changes

.prettierrc Outdated Show resolved Hide resolved

wmertens force-pushed the prettier branch from 4938330 to b8d381e Compare April 27, 2020 15:19

campersau requested changes Apr 29, 2020

View reviewed changes

wmertens force-pushed the prettier branch 2 times, most recently from 7f09a77 to e753177 Compare May 3, 2020 20:39

wmertens force-pushed the prettier branch from 4fbf098 to bbfe02b Compare May 4, 2020 05:51

campersau mentioned this pull request May 5, 2020

Prepare version 1.5.7 #1349

Merged

wmertens force-pushed the prettier branch 2 times, most recently from 6f0328f to 293ca73 Compare May 6, 2020 14:19

campersau requested changes May 8, 2020

View reviewed changes

package.json Outdated Show resolved Hide resolved

Gruntfile.js Outdated Show resolved Hide resolved

wmertens force-pushed the prettier branch 2 times, most recently from c4d08a4 to 6712526 Compare May 8, 2020 15:14

campersau approved these changes May 8, 2020

View reviewed changes

Hirse reviewed May 8, 2020

View reviewed changes

wmertens force-pushed the prettier branch from 6712526 to 1df35ec Compare May 9, 2020 07:16

wmertens added 5 commits May 11, 2020 14:36

prettier: add and configure

7163e8f

fix html errors

242eaa8

prettier refuses to format these

jslint: ignore prettier formatting

276c489

prettier: ensure lf line endings on windows

b4eeb22

It should be the default but --check complains

Move prettier check into grunt

49c0194

campersau force-pushed the prettier branch from 1df35ec to b302662 Compare May 11, 2020 12:39

prettier

b302662

campersau merged commit ac32c67 into FredrikNoren:master May 11, 2020

campersau mentioned this pull request Jul 1, 2020

Prepare version 1.5.8 #1391

Merged

wmertens deleted the prettier branch February 11, 2021 19:22

maximejf42 mentioned this pull request May 13, 2022

[Snyk] Fix for 1 vulnerabilities maximejf42/ungit#28

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Prettier #1316

Use Prettier #1316

wmertens commented Apr 23, 2020

ylecuyer commented Apr 23, 2020

wmertens commented Apr 23, 2020

Hirse left a comment

wmertens commented Apr 24, 2020

wmertens commented Apr 24, 2020

wmertens commented Apr 24, 2020

wmertens commented Apr 25, 2020

wmertens commented Apr 25, 2020

Hirse Apr 26, 2020

wmertens Apr 26, 2020

jung-kim Apr 28, 2020

wmertens May 4, 2020

wmertens commented Apr 27, 2020

jung-kim commented Apr 28, 2020

Hirse commented Apr 28, 2020

wmertens commented Apr 28, 2020

campersau left a comment

campersau Apr 29, 2020

wmertens commented May 4, 2020 •

edited

Loading

campersau left a comment

wmertens commented May 8, 2020

campersau left a comment

wmertens commented May 8, 2020

campersau commented May 8, 2020

Hirse May 8, 2020

campersau May 8, 2020

wmertens May 9, 2020

Hirse May 8, 2020

Hirse commented May 8, 2020

wmertens commented May 9, 2020

Hirse commented May 9, 2020

Use Prettier #1316

Use Prettier #1316

Conversation

wmertens commented Apr 23, 2020

ylecuyer commented Apr 23, 2020

wmertens commented Apr 23, 2020

Hirse left a comment

Choose a reason for hiding this comment

wmertens commented Apr 24, 2020

wmertens commented Apr 24, 2020

wmertens commented Apr 24, 2020

wmertens commented Apr 25, 2020

wmertens commented Apr 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wmertens commented Apr 27, 2020

jung-kim commented Apr 28, 2020

Hirse commented Apr 28, 2020

wmertens commented Apr 28, 2020

campersau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wmertens commented May 4, 2020 • edited Loading

campersau left a comment

Choose a reason for hiding this comment

wmertens commented May 8, 2020

campersau left a comment

Choose a reason for hiding this comment

wmertens commented May 8, 2020

campersau commented May 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hirse commented May 8, 2020

wmertens commented May 9, 2020

Hirse commented May 9, 2020

wmertens commented May 4, 2020 •

edited

Loading