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.

Sign up for GitHub

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

Jump to bottom

refactor(linting): use zero-config ts-standard #2198

Closed
wants to merge 1 commit into from
Closed

refactor(linting): use zero-config ts-standard #2198

wants to merge 1 commit into from

Conversation

LoneRifle
Copy link
Contributor

Context

Fixes #2197

Approach

  • Move to ts-standard, and remove mentions of eslint and prettier
  • Rework and relint the codebase to adopt ts-standard
  • Add build directory as explicit ignore in package.json files to help ts-standard

lamkeewei
lamkeewei approved these changes Dec 6, 2022
View reviewed changes
Copy link
Contributor

@lamkeewei lamkeewei left a comment

Choose a reason for hiding this comment

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

lgtm(?) Some minor comments on places where it is weird.

This works...but...is this how we want to format our code? We're losing things like import sorting.

frontend/src/setupTests.ts
@@ -6,11 +6,11 @@ import '@testing-library/jest-dom'

// Handle TypeError: env.window.matchMedia is not a function
window.matchMedia =
window.matchMedia ||
window.matchMedia ??
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering if this is ok here? Given that ?? and || are not exactly equivalent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should be okay? window.matchMedia is expected to be a function, so it will either be that, or undefined or null, but never the other falsy values

frontend/src/features/health/queries.ts
const { data } = useQuery(
['health'],
() => api.url(`/health`).get().json<HealthDto>(),
// 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.

Why do we need to disable the next line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got tired fighting the linter

backend/src/auth/auth.controller.ts
@Res() res: Response,
@Body() generateOtpDto: GenerateOtpDto,
@Body() generateOtpDto: GenerateOtpDto
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@Body() generateOtpDto: GenerateOtpDto
@Body() generateOtpDto: GenerateOtpDto

This one seems odd to format it in this manner (indenting subsequent decorators)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ts-standard quirkiness I'm afraid

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See typescript-eslint/typescript-eslint#1824

backend/src/auth/auth.controller.ts Outdated Show resolved Hide resolved
backend/src/config/config.schema.ts
}

addFormats({
'required-string': {
validate: (val?: string): void => {
if (val == undefined || val === '') {
if (val === undefined || val === null || val === '') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (val === undefined || val === null || val === '') {
if (!val) {

Would this be equivalent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

strict-boolean-expressions, unfortch

backend/src/database/entities/session.entity.ts
@@ -4,15 +4,15 @@ import { Column, DeleteDateColumn, Entity, Index, PrimaryColumn } from 'typeorm'
@Entity({ name: 'sessions' })
export class Session implements ISession {
@PrimaryColumn('varchar', { length: 255 })
id!: string
id!: string
Copy link
Contributor

Choose a reason for hiding this comment

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

Weird formatting here...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See typescript-eslint/typescript-eslint#1824

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See also the standard core team's response at mightyiam/eslint-config-love#978, and related issue at standard/standard#1872

@LoneRifle LoneRifle force-pushed the refactor/lint/ts-standard branch 2 times, most recently from bcc8287 to 174b175 Compare December 6, 2022 08:44
@LoneRifle LoneRifle force-pushed the refactor/lint/ts-standard branch from 174b175 to e9785d8 Compare December 8, 2022 05:46
@karrui
Copy link
Contributor

karrui commented Jan 4, 2023

This died due to #2360 hehe rip 🪦

@LoneRifle
Copy link
Contributor Author

Closing this for now; here's hoping we never have to visit zero-config linting ever again, in favour of our own eslint config and some ground rules about never ever touching lint config files

@LoneRifle LoneRifle closed this Mar 5, 2023
@LoneRifle LoneRifle deleted the refactor/lint/ts-standard branch March 5, 2023 07:33
@timotheeg
Copy link
Contributor

Hong mentioned zero config just last week again, so I don't think it's going away.

Apologies we didn't get to merge this, best to bite the bullet and just do it.

@LoneRifle
Copy link
Contributor Author

Actually, I have a plan...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lamkeewei lamkeewei lamkeewei approved these changes

@timotheeg timotheeg Awaiting requested review from timotheeg

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Explore zero-config linting
4 participants
@LoneRifle @karrui @timotheeg @lamkeewei