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

Renable prettier and fix all issues related to it #859

Merged
merged 2 commits into from Jul 12, 2017

Conversation

Projects
None yet
5 participants
@kunall17
Contributor

kunall17 commented Jul 12, 2017

No description provided.

@smarx

This comment has been minimized.

smarx commented Jul 12, 2017

Automated message from Dropbox CLA bot

@kunall17, it looks like you've already signed the Dropbox CLA. Thanks!

@kunall17 kunall17 changed the title from Renable prettier and fix all issues related to it to [WIP] Renable prettier and fix all issues related to it Jul 12, 2017

@kunall17 kunall17 changed the title from [WIP] Renable prettier and fix all issues related to it to Renable prettier and fix all issues related to it Jul 12, 2017

@zulipbot

This comment has been minimized.

Member

zulipbot commented Jul 12, 2017

Heads up @kunall17, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

@borisyankov

Everything looks ok, besides conforming to React Native formatting.

@zulipbot zulipbot added reviewed and removed needs review labels Jul 12, 2017

@borisyankov

This comment has been minimized.

Contributor

borisyankov commented Jul 12, 2017

Download a big file, for example:
https://github.com/facebook/react-native/blob/master/Libraries/Lists/FlatList.js
It has already been run past their 'prettier'.

@borisyankov

This comment has been minimized.

Contributor

borisyankov commented Jul 12, 2017

Prettier gets some of the config from eslint, so make sure the eslint rules match.

For example add this:
"comma-dangle": ["error", "always"],

@borisyankov

This comment has been minimized.

Contributor

borisyankov commented Jul 12, 2017

And I think, remove "arrow-parens": 0,.

@borisyankov

This comment has been minimized.

Contributor

borisyankov commented Jul 12, 2017

Another file from another repo to use:
https://raw.githubusercontent.com/facebook/react/master/src/renderers/dom/fiber/ReactDOMFiberEntry.js

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Jul 12, 2017

I ran the prettier with our config on the FlatList file and got this result kunall17@2aae2a2

@borisyankov

This comment has been minimized.

Contributor

borisyankov commented Jul 12, 2017

Yeah, I compared the results. The two suggestions I made are moving the formatting in the right direction, but the exercise to find few more are left to you ;)

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Jul 12, 2017

This is the config they use,

"prettier/prettier": ["error", {
  "singleQuote": true,
  "trailingComma": "all",
  "bracketSpacing": false,
  "jsxBracketSameLine": true,
  "parser": "flow"
}]

The difference's are the bracketSpacing, trailingComma and they use 80 printWidth and we use 100.
bracketSpacing I used as we used previous and about the trailingComma using all appends commas at weird places like kunall17@2aae2a2#diff-0e9e178f26b4b38fb1dd2bbb7076b554L451

@borisyankov

This comment has been minimized.

Contributor

borisyankov commented Jul 12, 2017

I don't think we necessarily need the squished {} with no spaces.
And also we don't need to match it 100%
I am just stressing any messing with the formatting better be done once, not sure we want to have another commit like this.

@borisyankov

This comment has been minimized.

Contributor

borisyankov commented Jul 12, 2017

Yeah, the trailing commas is OK. Formatting in many projects tends to move to adding this extra and not needed comma.

@borisyankov

This comment has been minimized.

Contributor

borisyankov commented Jul 12, 2017

So when you have

{
  a: 1,
  b: 1,
};

if you add another line it will not diff the b: 1, since you will not need to add a coma.

@borisyankov

This comment has been minimized.

Contributor

borisyankov commented Jul 12, 2017

BTW I am fine with the 100 character width, but also with 80 if we decide to lower it.
What do you think? And what does @Sam1301 think?

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Jul 12, 2017

I would highly suggest for 100, as I think most of the screen of users are big enough or high resolution to have 100 chars of code in a line in their view, and is more convenient to be viewed like this + a lot of code changes will be there if we go for 80!

And I think, remove "arrow-parens": 0,.

Then it would create conflict's b/w prettier
Btw was just going through the issue of arrow-parens in the prettier repo, and this is being highly suggested there to be implemented as an option.

@zulipbot zulipbot added needs review and removed reviewed labels Jul 12, 2017

@Sam1301

This comment has been minimized.

Member

Sam1301 commented Jul 12, 2017

I too think using printWidth 100 would be better because of the changes it will require in the existing files. Though one reason to go with print width 80 would be to be able to easily view files in a standard sized terminal.

@borisyankov borisyankov merged commit 6a64cd6 into zulip:master Jul 12, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.004%) to 41.241%
Details

@zulipbot zulipbot removed the needs review label Jul 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment