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

Add prettier code formatting #314

Merged
merged 13 commits into from
Feb 1, 2019
Merged

Add prettier code formatting #314

merged 13 commits into from
Feb 1, 2019

Conversation

jason-fox
Copy link
Contributor

Adds prettier and husky. All JavaScript files are autoformated on commit using the Git Hooks hook.

running npm run prettier will format all files with standardized whitespace.

@fgalan
Copy link
Member

fgalan commented Jan 30, 2019

I didn't realize looking to telefonicaid/iotagent-node-lib#753 but it is more evident in this one.

Current code style length limit is 120, which is pretty standard nowdays, e.g default in Pycharm IDE, used by github in its file views (actually github uses 130 as far as I have checkd), etc. Default 80 is too limitating, specially when each new intentation block "eats" 4 whispaces.

Thus, I'd suggest to add --print-width 120 to the CLI.

Let's also include @cblanco in this conversation to know his opinion (not specifically on the line length topic but in general about this PR).

@fgalan
Copy link
Member

fgalan commented Feb 1, 2019

@jason-fox we have just merged two big PRs (#305 and #316). So, could you update this PR with master so we can merge it?

There are other pending PR in this repo but I think they don't involve big issues:

Thanks!

package.json Outdated
@@ -23,17 +24,21 @@
"test": "mocha --recursive 'test/**/*.js' --reporter spec --timeout 3000 --ui bdd --exit",
"test:watch": "npm run test -- -w ./lib",
"lint": "jshint lib/ --config .jshintrc && jshint test/ --config test/.jshintrc",
"prettier": "prettier --tab-width 4 --print-width 120 --single-quote --trailing-comma es5 --write ./**/**/*.js **/*.js *.js",
Copy link
Member

Choose a reason for hiding this comment

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

Taking into account .prettierrc.json I understand that --tab-width 4 --print-width 120 --single-quote --trailing-comma es5 can be removed (as it has been done in lint-staged below).

Copy link
Contributor Author

@jason-fox jason-fox Feb 1, 2019

Choose a reason for hiding this comment

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

Fixed a583d49

Merge commit '2e8b2bad3174044fc3db1bfd0d2c398f75a628bb' into feature/prettier
Copy link
Member

@fgalan fgalan left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

Passing the ball to @cblanco for additional LGTM before merging.

@@ -13,7 +13,7 @@
"maxparams": 6,
"maxdepth": 4,
"camelcase": true,
"maxlen": 120,
"maxlen": 180,
Copy link
Member

Choose a reason for hiding this comment

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

Why this change is needed?

I mean, if prettier is ensuring "printWidth": 120, why the linter needs to extend +60 chars the limit?

Just curiosity...

Copy link
Contributor Author

@jason-fox jason-fox Feb 1, 2019

Choose a reason for hiding this comment

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

@fgalan, prettier is attempting to fit within 120 characters, but according to its own opinionated rules it won't always succeed particularly if you have a line with a long string. Prettier will always remove unnecessary concatenations in strings, and always indent properly (with a tab 4) so if you are left with a line over 120 characters long jslint will complain.

We're fortunate that we never need very long strings in the code base then. However each of the tests does contain a single lengthy string in it - the description of the test itself - also this ends up being prettily indented by about 4 tabs.

Rather than "fixing" this by truncating the description text, I chose to relax the jslint rule for the tests only.

Of course if you leave the default tab at 2 spaces you're less likely to reach the limit.

Copy link
Member

Choose a reason for hiding this comment

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

I think is a good solution. No problem at all. Thanks!

@cblanco
Copy link
Contributor

cblanco commented Feb 1, 2019

LGTM. Good Work!

@fgalan fgalan merged commit 4f7419e into telefonicaid:master Feb 1, 2019
@fgalan
Copy link
Member

fgalan commented Feb 1, 2019

PR #309, authored by @Secmotic. I think is pretty small

Fortunatelly, no conflict seems to happen on #309 after merging this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants