prettier integration POC #360

Undistraction · 2018-02-11T16:57:19Z

This PR adds prettier and integrates it with eslint. It's for discussion so please take a look.

I have run it on all files in the src/ and test/ dirs.

Prettier's minimal options are here. I would recommend:

Drop the 'Print Width' to 80.
Lose the semi-colons.
Use backticks instead of single quotes.

I have it configured (with VSCode) so it formats on save which I find to be very very useful. It really becomes part of your workflow - you don't need to think about formatting at all.

I've added it to the lint script. It could go into the lint:fix script instead, but I think it should happen transparently when linting.

It'll take some tweaking to get eslint happy with what prettier does, but I'll hold off doing that for the moment until we decide it's something we want.

closes #336

char0n · 2018-02-11T19:34:55Z

Thank you for this POC.

Drop the 'Print Width' to 80.
I'd go for that. Let see what it will do to our jsdoc comments that are pretty long

Lose the semi-colons.
Don't trust ASI. I'd rather stay explicit with this one

Use backticks instead of single quotes.
We can switch later if there are reasonable amount of usecases.

Let's keep it on evolution level, not revolution ;]

Ok couple of things that I have questions about

) How does this play with our eslint config ? Parentheses around single parameter arrow functions / Add support for arrow-parens property prettier/prettier#812

2.) We will loose 2 lines after import statements; but I can live with that (prettier/prettier#1610)

3.) This is something that really sucks

4.) Spaces in functions declarations: prettier/prettier#3847; Is there some options for this ?

Undistraction · 2018-02-11T21:08:32Z

Don't trust ASI. I'd rather stay explicit with this one

This is part of what prettier does. With semis disabled it will still insert semis for cases where their absence would cause problems.

I'd go for that. Let see what it will do to our jsdoc comments that are pretty long

I'll take a look tomorrow

We can switch later if there are reasonable amount of usecases.

Np

How does this play with our eslint config

Prettier will win, so its a matter of changing eslint rules to be harmonious with how prettier formats and disabling all eslint autofixing. When we are happy with how prettier is formatting I can look at this.

We will loose 2 lines after import statements

Yep. This is not optional

This is something that really sucks

Not sure exactly what you mean - the wrapping?

Spaces in functions declarations

Looks like this is going to land shortly: prettier/prettier#3903

char0n · 2018-02-12T07:48:24Z

This is part of what prettier does. With semis disabled it will still insert semis for cases where their absence would cause problems.

Ok understand, let's keep the semicolons in for now.

Not sure exactly what you mean - the wrapping?

Wrapping of chained method calls. Seems really unreadable for me. Let's see how will it look at 80 chars.

# Conflicts: # src/index.d.ts # test/mapIndexed.js

Undistraction · 2018-02-12T16:03:08Z

Got everything playing nicely locally so please take a look when you get the chance. Prettier now runs prior to eslint and tslint on npm run lint.

Looks like code climate has two issues for isArrayLike. Not sure why they are showing now, but can't argue. That is one ugly function ;)

If everything else is OK I'll create a separate PR for isArrayLike that reduces the use of return and get that in first, then we can merge this one.

Undistraction · 2018-02-13T21:48:37Z

If everything else is OK I'll create a separate PR for isArrayLike that reduces the use of return and get that in first, then we can merge this one.

@char0n Shall I get this done before this PR gets too out of sync with master?

char0n · 2018-02-14T21:24:42Z

I'll look at this during tomorrow. Meanwhile I will not merge any code related or configuration PRs. Sorry for delay

char0n

LGTM. Great job! Some constructrs look a little bit strange that is price for consistency I guess.

char0n · 2018-02-15T19:27:38Z

.eslintrc

  ],
-  "plugins": ["eslint-plugin-ramda"],
+  "plugins": ["eslint-plugin-ramda", 


properly format the array comprehension pls

char0n · 2018-02-15T19:30:53Z

package.json

@@ -57,8 +57,7 @@
  },


please drop the package-lock.json

char0n · 2018-02-15T19:42:45Z

test/isNotFloat.js

    eq(RA.isNotFloat(0), true);
    eq(RA.isNotFloat(1), true);
    eq(RA.isNotFloat(-100000), true);
    eq(RA.isNotFloat(MAX_SAFE_INTEGER), true);
    eq(RA.isNotFloat(MIN_SAFE_INTEGER), true);
-    eq(RA.isNotFloat(5e+0), true);
+    eq(RA.isNotFloat(5), true);


the scientific notation has been dropped here, but was here by design, can we ignore here ?

char0n · 2018-02-15T19:42:52Z

test/isNotFloat.js


    eq(RA.isNotFloat(0.1), false);
    eq(RA.isNotFloat(Math.PI), false);
-    eq(RA.isNotFloat(5.56789e+0), false);
+    eq(RA.isNotFloat(5.56789), false);


the scientific notation has been dropped here, but was here by design, can we ignore here ?

char0n · 2018-02-15T19:54:00Z

We can fix code climate issues after the merge. There are some conflicts to resolve in this PR. Can you check it out before we merge (by rebasing on top of currenct master) ? I'd like to rebase this instead of merging.

char0n · 2018-02-16T22:02:24Z

Rerurning the ci build and if green, then merging.

I can see now that there are 4 pending comments that has not been addressed. Can you look at it pls ?

Undistraction · 2018-02-16T22:55:50Z

@char0n Those should now be addressed.

Undistraction added 2 commits February 11, 2018 16:46

build(prettier): add and integrate with eslint

d33f039

style(prettier): run prettier on src/ and tests/

37a6efb

Undistraction changed the title ~~prettier integration~~ prettier integration POC Feb 11, 2018

Undistraction added 4 commits February 12, 2018 15:47

build(prettier/tslint/eslint): Iron out conflicts

1a16c75

style(prettier): reformat with new settings

1c6a345

Merge branch 'master' into undistraction/prettier

590904d

# Conflicts: # src/index.d.ts # test/mapIndexed.js

style(prettier): rerun

c6b7e79

char0n approved these changes Feb 15, 2018

View reviewed changes

Merge branch 'master' into undistraction/prettier

6092300

Undistraction added 4 commits February 16, 2018 23:46

Merge branch 'master' into undistraction/prettier

d3a5b89

refactor(prettier): ignores for isNotFloat tests

2bd6887

style: formatting of eslintrc

82ebcf4

build: remove stray yarn package-lock.json from vc

8417281

refactor(prettier): missed ignore in isFloat tests

ced22fa

char0n merged commit a3109fc into char0n:master Feb 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

prettier integration POC #360

prettier integration POC #360

Undistraction commented Feb 11, 2018 •

edited

Loading

char0n commented Feb 11, 2018

Undistraction commented Feb 11, 2018

char0n commented Feb 12, 2018

Undistraction commented Feb 12, 2018 •

edited

Loading

Undistraction commented Feb 13, 2018

char0n commented Feb 14, 2018

char0n left a comment

char0n Feb 15, 2018

char0n Feb 15, 2018

char0n Feb 15, 2018

char0n Feb 15, 2018

char0n commented Feb 15, 2018

char0n commented Feb 16, 2018 •

edited

Loading

Undistraction commented Feb 16, 2018

prettier integration POC #360

prettier integration POC #360

Conversation

Undistraction commented Feb 11, 2018 • edited Loading

char0n commented Feb 11, 2018

Undistraction commented Feb 11, 2018

char0n commented Feb 12, 2018

Undistraction commented Feb 12, 2018 • edited Loading

Undistraction commented Feb 13, 2018

char0n commented Feb 14, 2018

char0n left a comment

Choose a reason for hiding this comment

char0n Feb 15, 2018

Choose a reason for hiding this comment

char0n Feb 15, 2018

Choose a reason for hiding this comment

char0n Feb 15, 2018

Choose a reason for hiding this comment

char0n Feb 15, 2018

Choose a reason for hiding this comment

char0n commented Feb 15, 2018

char0n commented Feb 16, 2018 • edited Loading

Undistraction commented Feb 16, 2018

Undistraction commented Feb 11, 2018 •

edited

Loading

Undistraction commented Feb 12, 2018 •

edited

Loading

char0n commented Feb 16, 2018 •

edited

Loading