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 and reformat. #924

Closed
wants to merge 6 commits into from

Conversation

Overload119
Copy link
Contributor

This pull request accomplishes 2 things

  • Adds Prettier and the standard configuration for it.
  • Perform Prettier on 2 files, so you can see the changes it makes.
  • Adds some comments to ConfigManager which I plan on adding to (in order to sync to disk to avoid the issues upgrading from previous versions).

There are plenty of benefits of Prettier that you can read by reading on the project; in my opinion the best things are

  • You don't have to argue about code during PR, making it easier to get up and running with making contributions.
  • It adds a consistent style guide and easier on the eyes with so many new developers touching the code base

I didn't want to run it on the whole codebase yet; instead I think we should migrate over time with the files we end up touching to keep the PRs still reviewable.

I currently setup Atom to auto-prettier on save; I would encourage the same for contributors.

@sosukesuzuki
Copy link
Member

sosukesuzuki commented Sep 30, 2017

Hi! @Overload119 ! Thank you for creating the pull request!
Now, in Boostnote javascript is unified with semicolon less. I'm wondering why you put on a semicolon.

@Overload119
Copy link
Contributor Author

This is actually customizable, so if we want to change it we can:

https://github.com/prettier/prettier#semicolons

However, I think in general we should pick the code style that conforms to what more JS contributors across GitHub would prefer; there's an overwhelming majority of developers use semicolons.

image

(from https://hackernoon.com/what-javascript-code-style-is-the-most-popular-5a3f5bec1f6f)

@sosukesuzuki
Copy link
Member

Thank you for telling me it.
We think that it is important for more developers to create an environment that is easy to contribute. So, we will discuss it!

@KLszTsu
Copy link
Contributor

KLszTsu commented Oct 1, 2017

I think using semicolons or not is just a habit of developers. Its being or not being are both fine:

  • A code without semicolons won't be harder to read than that with semicolons.
  • Not using semicolons won't cause errors.

If a code is unified with one style,

Now, in Boostnote javascript is unified with semicolon less.

then I don't think it will have problems.

Thanks.

@asmsuechan
Copy link
Contributor

asmsuechan commented Oct 1, 2017

I don't think this change is necessary by following reasons.

  1. eslint already exists
  2. if this PR is mergeable, I've already run eslint --fix
  3. this PR has too much impact on codebase so there's no guarantee to work the application properly (sadly, there's no test for components)

However, thank you for your PR.

@Overload119
Copy link
Contributor Author

@asmsuechan thanks of the discussion! I would say this change may not be necessary, but I listed the advantages above.

  1. This tool has different goals than ESLint.

Linters have two categories of rules:

Formatting rules: eg: max-len, no-mixed-spaces-and-tabs, keyword-spacing, comma-style...

Prettier alleviates the need for this whole category of rules! Prettier is going to reprint the entire program from scratch in a consistent way, so it's not possible for the programmer to make a mistake there anymore :)

Code-quality rules: eg no-unused-vars, no-extra-bind, no-implicit-globals, prefer-promise-reject-errors...

Prettier does nothing to help with those kind of rules. They are also the most important ones provided by linters as they are likely to catch real bugs with your code!

  1. (I'm not sure why the argument is here, perhaps you can clarify?)
  2. The module has and probably has been run collectively on millions of files; it has recently surpassed v1.0; the reason I introduced it little by little (in this PR) it has no impact on the entire codebase, but yes, at some point if it was run on the entire codebase it would change a lot of things -- however, since it transforms the AST you are guaranteed to get runnable code; so there is actually very little risk.

We have used this module on tens of thousands of file at Facebook, so I don't think we will have any issue here.

I hope we can continue discussion!

@asmsuechan
Copy link
Contributor

Hi, @Overload119.

  1. As for code style, eslint includes works of prettier, right? I think there are Formatting rules in eslint.
  2. I meant we've considered changing codebase but we didn't (well, you don't need to reply this section because it's our issue)
  3. According to Use Prettier facebook/react#9101, it seems there's little probability that any issues happen when the code is changed by prettier. That's why I mentioned there are risks is that because I've experienced changes by a lint tool made me bugs due to running orders in Ruby. However, I want to change them after we get tests...

MY OPINION:
I don't care about prettier if we run it on each release. However, I don't want to add it for now because of our release flow and tests. So I want to add it after we prepare them.

I understand your opinions. I have to ask them on committers.

@kazup01 kazup01 added the discussion 💬 Issue concerns a discussion. label Nov 7, 2017
@kazup01
Copy link
Member

kazup01 commented Dec 2, 2017

I close it because there are many conflicts. If someone want to reopen, please make other pull request.
Thank you for your suggestion @Overload119 .

@kazup01 kazup01 closed this Dec 2, 2017
@kazup01 kazup01 removed the discussion 💬 Issue concerns a discussion. label Dec 2, 2017
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.

None yet

5 participants