Skip to content
This repository has been archived by the owner on Jan 14, 2020. It is now read-only.

Use eslint for finer control of code format #225

Merged
merged 30 commits into from
Jun 7, 2018
Merged

Conversation

tyleryasaka
Copy link
Contributor

@tyleryasaka tyleryasaka commented Jun 5, 2018

Checklist:

  • Code contains relevant tests for the problem you are solving
  • Ensure all new and existing tests pass
  • Update any relevant READMEs and docs
  • Submit to the develop branch instead of master

Description:

Switches us from using just prettier to using eslint + prettier

I switched to eslint because it is much more configurable, and I have had a great experience it with it on another project.

We now have:

  • Enforced consistency over var/const/let. Followed style suggested in this article. Also see const vs let vs var #192.
  • Consistent use of quotes (double single or backticks)
  • Lots of other nice restrictions that are enabled by eslint out of the box, like preventing unused vars

.eslintrc.js Outdated
semi: ["error", "never"],
quotes: [
"error",
"double",
Copy link
Member

Choose a reason for hiding this comment

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

double quotes 😫

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@micahalcorn Do you prefer single quotes? I'm not opinionated on it, I just chose double because more of our code was using double. 😃

Copy link
Member

Choose a reason for hiding this comment

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

I vote for single quotes too :-)

let [
listingAddress,
const [
,
Copy link
Member

@micahalcorn micahalcorn Jun 5, 2018

Choose a reason for hiding this comment

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

Are these now rogue commas? 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@micahalcorn Nah, it's just destructuring syntax for ignoring values we don't care about. See: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment (section "Ignoring some returned values")

const IN_DISPUTE = 4 // We are in a dispute
const REVIEW_PERIOD = 5 // Time for reviews (only when transaction did not go through)
// const IN_DISPUTE = 4 // We are in a dispute
// const REVIEW_PERIOD = 5 // Time for reviews (only when transaction did not go through)
Copy link
Member

Choose a reason for hiding this comment

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

Reviews ended being parts of 2️⃣and 3️⃣. Should this just be removed completely, or are we planning to bring back a post-dispute review period @DanielVF?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Imho we don't have to make that decision now - that's why I just commented them out, so we don't have to deal with it in this PR. (I think it's better to just keep this focused on the linter change.)

@micahalcorn
Copy link
Member

I'll defer to @DanielVF on some of this. I certainly favor const > let > var and generally eslint. 👍

@jordajm
Copy link
Contributor

jordajm commented Jun 5, 2018

I really like eslint too @tyleryasaka - thanks for working on this.

One suggestion would be to consider using the jsx-quotes rule to enforce use of double quotes in JSX attributes (which is similar of HTML, where double quoting is more conventional) while staying with single quotes in the JavaScript (which seems to be the standard throughout the app so far, and is definitely my personal preference)
https://eslint.org/docs/rules/jsx-quotes
Thoughts on that?

^^^ haha - oops - thought this was in the demo-dapp repo 🤦‍♂️

@tyleryasaka
Copy link
Contributor Author

@jordajm Once this gets merged I can do a similar PR in demo dapp repo!

.eslintrc.js Outdated
@@ -0,0 +1,35 @@
module.exports = {
Copy link
Member

Choose a reason for hiding this comment

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

Can we put this in package.json instead of another dotfile?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -62,7 +60,6 @@
"mocha": "^5.1.1",
"mocha-loader": "^1.1.3",
"node-watch": "^0.5.8",
"prettier": "1.12.1",
Copy link
Member

Choose a reason for hiding this comment

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

Does eslint do automatic file formatting? In other projects I've used both prettier and eslint together without issues

Copy link
Contributor Author

@tyleryasaka tyleryasaka Jun 5, 2018

Choose a reason for hiding this comment

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

@nick Yeah eslint does file formatting. Eslint does everything we need it to do - I don't see any use for prettier.

npm run format still fixes all the files in place (to the extent that it can be done automatically).

Copy link
Member

Choose a reason for hiding this comment

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

Apparently prettier is more powerful than eslint at formatting files. Perhaps we can run both eslint and prettier separately on demo-dapp, see which does a better job and decide to dump prettier based on that?

@DanielVF
Copy link
Collaborator

DanielVF commented Jun 5, 2018

I prefer const > let > var as well.

const does catch errors from time to time. I don't like it, since it makes half my lines longer than the should be and harder to read, but the utility is undoubtable, and so I use it anyway.

Prettier as done a great job of just automatically working and bringing the code into formatting spec. There a big difference between a tool you just run and it works, vs something that spits out lists of spacing errors that you have to go fix yourself - which has always been my past experience with eslint. (Has it gotten better?)

@ambertch
Copy link
Contributor

ambertch commented Jun 5, 2018 via email

@tyleryasaka
Copy link
Contributor Author

tyleryasaka commented Jun 5, 2018

@DanielVF eslint does automatic formatting as well (npm run format does the same thing as before); there are just some issues that can't be fixed automatically. I don't see any advantages of prettier over this. We're just seeing more errors because eslint is stricter - which is good, imho.

The --fix option does in-place fixing (when it can be done automatically). See:

"format": "npx eslint --fix 'src/**/*.js' 'test/**/*test.js' 'contracts/test/**/*.js' 'scripts/**/*.js'",

Edit: Just saw @nick 's comment. I'll try setting up prettier and eslint side by side then.

Per @nick 's request; removes the need for yet another dot file.
This layers prettier on top of eslint for better in-place code formatting.
@tyleryasaka
Copy link
Contributor Author

Updated to use prettier-eslint-cli

@tyleryasaka
Copy link
Contributor Author

tyleryasaka commented Jun 5, 2018

Single quotes it is! The vote is 2:1 in favor of single quotes. (I abstained but if I had to pick I also like single quotes - easier for me to read and type.)

@tyleryasaka
Copy link
Contributor Author

Next up: spaces vs tabs... jk jk

@tyleryasaka
Copy link
Contributor Author

@micahalcorn I believe I have addressed all the concerns raised here. How do you feel about merging this? (Good to merge as quickly as possible to reduce conflicts.)

@micahalcorn micahalcorn merged commit d252327 into develop Jun 7, 2018
@micahalcorn micahalcorn deleted the tyleryasaka/eslint branch June 7, 2018 16:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants