sealcode / sealious

An extensible, declarative node framework
25 stars 2 forks source link

Eslint #277

Closed adwydman closed 8 years ago

adwydman commented 8 years ago

Firstly I'm sorry for the commit names, I was testing if pre-commit works.

I am creating a pull request for final approval, as many things in the code are going to be changed.

kuba-orlik commented 8 years ago

The commit messages are... embarassing ;p. Please rebase the branch so they're squashed into one, or at least change their messages into something meaningful :)

Also, reviewing 1000+ changed is a daunting task. Is there a possibility you could divide the changes into separate commits, like "Add semicolons", "Change var to const" etc?

Thirdly, didn't we agree on not changing var -> const/let in old codebase for now?

adwydman commented 8 years ago

Please rebase the branch so they're squashed into one, or at least change their messages into something meaningful :)

Noted.

Also, reviewing 1000+ changed is a daunting task. Is there a possibility you could divide the changes into separate commits, like "Add semicolons", "Change var to const" etc?

I'm not actually expecting anyone to review all of those changes, it's more about checking the ESLint config package to see if the rules there are sufficient.

Thirdly, didn't we agree on not changing var -> const/let in old codebase for now?

We agreed that it won't be a dedicated task, but only when encountering var when working on the code.

kuba-orlik commented 8 years ago

The config looks OK to me :) Once you rebase the branch so there are no redundant and weirdly-named commits, I'll be willing to merge it :)

adwydman commented 8 years ago

@kuba-orlik I renamed the commit titles, but I am afraid I might have broken something...

kuba-orlik commented 8 years ago

Why do you think so?

adwydman commented 8 years ago

I never used rebase, so maybe that's why :)

kuba-orlik commented 8 years ago

Is the result of npm test different than before rebase?

adwydman commented 8 years ago

No, it's the same - 128 passing.

kuba-orlik commented 8 years ago

Then I think it's unlikely you broke something with the code. Will check the commit graph status

adwydman commented 8 years ago

I had to create a separate branch eslint-temp, because I effed up something. Nevertheless, ESLint is now on references!