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

chore(eslint): upgrade eslint #15457

Merged
merged 18 commits into from Oct 18, 2019
Merged

chore(eslint): upgrade eslint #15457

merged 18 commits into from Oct 18, 2019

Conversation

asalem1
Copy link
Contributor

@asalem1 asalem1 commented Oct 17, 2019

Upgraded ESLINT and updated linter errors

typescript-eslint/typescript-eslint#389

@asalem1 asalem1 requested a review from a team October 17, 2019 20:22
@ghost ghost requested review from ebb-tide and removed request for a team October 17, 2019 20:22
@121watts 121watts requested a review from a team October 18, 2019 00:46
@121watts
Copy link
Contributor

121watts commented Oct 18, 2019

Pretty massive. Apologies. Mostly unneeded / improper use of awaits

Copy link
Contributor

@drdelambre drdelambre left a comment

Choose a reason for hiding this comment

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

some questions about side effects and a call to limit the scope of eslint exceptions

}
parser: '@typescript-eslint/parser',
parserOptions: {
project: './tsconfig.test.json',
Copy link
Contributor

Choose a reason for hiding this comment

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

😬 sorry

@@ -50,7 +50,9 @@ export class Signin extends PureComponent<Props, State> {

if (this.hasMounted) {
this.setState({loading: RemoteDataState.Done})
this.intervalID = setInterval(this.checkForLogin, FETCH_WAIT)
this.intervalID = setInterval(() => {
this.checkForLogin()
Copy link
Contributor

Choose a reason for hiding this comment

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

this is better!

@@ -1,6 +1,7 @@
// Libraries
import React, {SFC} from 'react'

/* eslint-disable no-mixed-spaces-and-tabs */
Copy link
Contributor

Choose a reason for hiding this comment

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

these should have a matching /* eslint-enable */ at the end of the file, in the name of compositional ease (changing include style, copy pasta, etc)

@@ -1,6 +1,7 @@
// Libraries
import React, {SFC} from 'react'

/* eslint-disable no-mixed-spaces-and-tabs */
Copy link
Contributor

Choose a reason for hiding this comment

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

needs a matching /* eslint-enable */

@@ -1,6 +1,7 @@
// Libraries
import React, {SFC} from 'react'

/* eslint-disable no-mixed-spaces-and-tabs */
Copy link
Contributor

Choose a reason for hiding this comment

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

needs a matching /* eslint-enable */

@@ -13,6 +13,7 @@ const setup = (override?) => {
...override,
}

// eslint-disable-next-line
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a specific rule we can add here?

@@ -26,10 +26,10 @@ export function findIncludedsFromRelationships<
export function findIncludedFromRelationship<
T extends {id: string; type: TemplateType}
>(
includeds: {id: string; type: TemplateType}[],
included: {id: string; type: TemplateType}[],
Copy link
Contributor

Choose a reason for hiding this comment

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

😲

@@ -1,3 +1,4 @@
/* eslint-disable */
Copy link
Contributor

Choose a reason for hiding this comment

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

needs a matching /* eslint-enable */

constructor() {
super(...arguments)
constructor(...args) {
super(...args)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@drdelambre drdelambre left a comment

Choose a reason for hiding this comment

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

...changing to comment in case i get busy.

@drdelambre drdelambre self-requested a review October 18, 2019 16:44
@drdelambre drdelambre dismissed their stale review October 18, 2019 16:45

watts promises to scope linting rules

@121watts 121watts changed the title chore(eslint): upgraded eslint chore(eslint): upgrade eslint Oct 18, 2019
Copy link
Contributor

@drdelambre drdelambre left a comment

Choose a reason for hiding this comment

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

get on with it

"webpack.tsconfig.json"
"webpack.tsconfig.json",
"webpack.*.ts",
"coverage"
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

// but we cannot because of the default system buckets
// since cypress selectors are so fast, that sometimes a bucket
// that is deleted will be selected before it gets deleted
cy.reload()
Copy link
Contributor

Choose a reason for hiding this comment

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

got it.

@drdelambre
Copy link
Contributor

GOOD LUCK WITH YOUR MERGE CONFLICTS EVERYONE

@121watts 121watts merged commit c720147 into master Oct 18, 2019
@121watts 121watts deleted the chore/eslint-upgrade branch October 18, 2019 18:40
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

4 participants