refactor: project wide clean up #83

epiqueras · 2018-02-27T06:49:55Z

This PR:

Adds linting and formatting to workflow.
Fixes linting errors.
Fixes some inconsistencies and duplicated or unused code (still some left).
Deletes unused files.
Organizes files.
Adds coverage reports.
Removes unused dependencies.
Streamlines Webpack config.

satello

A lot to look through but overall the consistency will be good even if I am not in love with all the linting rules. Is this the same linter we are using for our other projects?

satello · 2018-02-27T13:52:53Z

CHANGELOG.md

+
+<a name="0.0.61"></a>
+
+## 0.0.61 (2018-02-27)


you should be on 0.0.62 now

Why? Refactors don't bump versions, https://semver.org.

Sure but these changes weren't from 0.0.61 anyways right? Also changelogs are nice but I still think we are a little early since we really don't even have a stable v1 yet

Yeah I'm pretty sure those changes were included in that release.

nvm i see whats going on

satello · 2018-02-27T13:58:17Z

src/abstractWrappers/ArbitrableContract.js


    // return contract data
-    return await this.getData(contractInstance.address, account)
+    return this.getData(contractInstance.address, account)


Why remove await? I think it should return the data not a Promise

async functions return a promise, you're just adding another layer of promise for no reason.

What do you mean? It is the difference between returning a promise (which we don't do throughout the rest of the API) and returning the resulting data

@satello

No, async/await functions return a promise that resolves to the return value of the function body.

By adding that await, you are not accomplishing anything. You are waiting for that first promise internally before wrapping the resolve value in another promise and returning that. Either way, the consumer receives a promise that resolves to the same data.

I made this snippet to illustrate:

const getData = () => new Promise(resolve => setTimeout(() => resolve('Hello World'), 1000)); async function run() { return getData(); } async function runWithExtraPromise() { return await getData(); } run().then(res1 => console.log('Without extra promise: ', res1)); runWithExtraPromise().then(res2 => console.log('With extra promise: ', res2)); // res1 === res2

The extra await basically turns promise into promise.then(res => res).

satello · 2018-02-27T14:00:52Z

src/abstractWrappers/EventListeners.js

@@ -1,111 +1,142 @@
 import PromiseQueue from '../../util/PromiseQueue'
+


what linting option is this? I don't think we need lines between every import

It's https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/order.md.

It's a line between import 'groups'. It makes it a lot easier to see what you are importing and why.

I think parent and children imports should be in the same "group" but its ok

satello · 2018-02-27T14:01:50Z

src/abstractWrappers/EventListeners.js

    // key: contract address, value: true/false
-    this._arbitratorEventsWatcher
-    this._arbitrableEventsWatcher
+    // this._arbitratorEventsWatcher


It is good practice to declare class variables in the constructor even if technically it works just to use them later on

satello · 2018-02-27T14:38:06Z

src/abstractWrappers/Notifications.js


    if (_.isNull(notificationIndex)) {
-      throw new Error(`No notification with txHash ${txHash} exists`)
+      throw new TypeError(`No notification with txHash ${txHash} exists`)


I'm not sure I would call this a TypeError. Usually TypeError refers to a variable being the wrong type. Which I guess you could argue notificationIndex is but the error is more of a Runtime "Server" Error

Yeah, we can change this. I think errors need some refactoring in general. We are just catching errors everywhere only to throw them again.

satello · 2018-02-27T14:40:32Z

src/abstractWrappers/Notifications.js

    const txHash = event.transactionHash
-    const arbitrableData = await this._ArbitrableContract.getData(event.args._arbitrable)
+    const arbitrableData = await this._ArbitrableContract.getData(
+      event.args._arbitrable


I'm not sure I really understand this linting rule. I've seen functions being rolled up onto one line with 3 params and then there is something like this doing the opposite with 1. I think I prefer new line per param for all functions including declarations

It's about line width, see https://github.com/prettier/prettier.

Not my favorite one either but 👍

satello · 2018-02-27T14:50:44Z

src/abstractWrappers/Disputes.js

  ) => {
    if (!this._eventListener) return

+    const defaultAccount = account


whats your thinking here?

Something weird happens with the parser when a function param is only used as a default param in another function. It trips up for some reason.

satello · 2018-02-27T15:08:19Z

src/abstractWrappers/Notifications.js

    message = '',
    data = {},
-    read = false,
+    read = false


I'm not super picky on this linting rule because I think I am in the minority opinion in general but I prefer having extra commas at the end of every iterable. I think it is a simple way to avoid bugs

The linter will catch any missing commas. It's merely a style thing.

satello · 2018-02-27T15:17:20Z

src/contractWrappers/KlerosWrapper.js

-        ]
-      )
-
+      ;[voteCounters, status] = await Promise.all([


extra semicolon or is this needed?

Yes,

In general, \n ends a statement unless:

The statement has an unclosed paren, array literal, or object literal or ends in some other way that is not a valid way to end a statement, (For instance, ending with . or ,.).

The line is -- or ++, (in which case it will decrement/increment the next token.).

It is a for(), while(), do, if(), or else, and there is no {.

The next line starts with [, (, +, *, /, -, ,, ., or some other binary operator that can only be found between two tokens in a single expression.

The last one applies here so we need the explicit line break ;.

satello · 2018-02-27T15:20:19Z

src/kleros.js

  ) => {
    await this._storeWrapper.setUpUserProfile(account)
    this.eventListener.clearArbitratorHandlers()
    await this.disputes.addDisputeEventListener(arbitratorAddress, account)


Why not this one too?

Line width?

clesaege · 2018-02-27T15:40:59Z

For linting, can't we use an automatic program to save time?

epiqueras · 2018-02-27T16:21:56Z

@clesaege
Yes, you should use your editor's eslint plugin with formatOnSave turned on.
You can also run yarn run lint --fix.

Merges #83

epiqueras added 2 commits February 26, 2018 22:34

refactor: whole project

d9ff054

docs(readme): update readme

9954862

epiqueras requested a review from satello February 27, 2018 06:50

chore(dependencies): add webpack-cli

2302cc7

epiqueras added the refactor label Feb 27, 2018

epiqueras self-assigned this Feb 27, 2018

epiqueras added this to the February 26 -> March 5 milestone Feb 27, 2018

epiqueras added 2 commits February 26, 2018 23:04

fix: skip broken test assertions

f9480c0

chore(release): 0.0.61

8dfc204

satello previously approved these changes Feb 27, 2018

View reviewed changes

satello dismissed their stale review via b8d7a0b February 27, 2018 15:31

cleanup while reviewing

946d113

satello force-pushed the refactor/add-kleros-scripts branch from b8d7a0b to 946d113 Compare February 27, 2018 16:10

refactor(kleros-wrapper): remove unnecessary eslint ignore

a942f0b

satello merged commit 5e11e3f into develop Feb 27, 2018

epiqueras deleted the refactor/add-kleros-scripts branch February 27, 2018 16:49

This was referenced Feb 28, 2018

migrate to ganache #78

Closed

change deploy command with yarn #53

Closed

epiqueras added a commit that referenced this pull request Feb 28, 2018

feature(merge): event timestamps

5368407

Merges #83

		@@ -1,111 +1,142 @@
		import PromiseQueue from '../../util/PromiseQueue'

refactor: project wide clean up #83

refactor: project wide clean up #83

Uh oh!

Conversation

epiqueras commented Feb 27, 2018

Uh oh!

satello left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

epiqueras Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

satello Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clesaege commented Feb 27, 2018

Uh oh!

epiqueras commented Feb 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

epiqueras Feb 27, 2018 •

edited

Loading

satello Feb 27, 2018 •

edited

Loading