oppia / oppia

A free, online learning platform to make quality education accessible for all.
https://www.oppia.org
Apache License 2.0
5.83k stars 4.09k forks source link

Introduce `@typescript-eslint` rules that extend eslint rules #10616

Open vojtechjelinek opened 4 years ago

vojtechjelinek commented 4 years ago

@typesctript-eslint contains a subset of rules that extend basic rules provided by eslint, these rules usually work better and provide additional checks. The checks that are these extend-rules are listed in ESLint typescript rules at the bottom (the rules that have a reddish background).

The goal of this issue should be to implement these rules, starting with the ones that we use already but only the simple eslint version and then all the other ones that can also be enabled, some of these might help us to get rid of our own listing.

For some of these there might be one more problem that we do not have all files covered with tsconfig.json. And some of these need a decision on which style we want to use (this is visible in the sheet)

Steps to follow to create a PR:

  1. Ask @vojtechjelinek to assign you to one of the issue (Assuming got assigned to "@typescript-eslint/<rule>)
  2. Add "@typescript-eslint/<rule>: [...]` in the .eslintrc file.
  3. Run python -m scripts.linters.pre_commit_linter --only-check-file-extensions=js --path=. --verbose to check existing issues in the codebase. (This can take 3-5 mins mins to complete.)
  4. If the above script takes too much time or you have a slow machine then consider pushing the code to GitHub and GitHub actions will run these checks for you.
  5. Fix all the issues found by running the above command.
  6. Commit all the changes and create a PR.

List of rules to enable:

sajalasati commented 3 years ago

@vojtechjelinek Is this issue ready to start working on? Based on the description I understand that most of the checks are already implemented in eslint, and we only need to enable them, so do you think this becomes a good candidate for good-first-issue tag?

vojtechjelinek commented 3 years ago

@sajalasati Yes, although it needs bit better description (I don't have the bandwidth to do it rn) and some check might require some decision (like which style to use).

sajalasati commented 3 years ago

@sajalasati Yes, although it needs bit better description (I don't have the bandwidth to do it rn) and some check might require some decision (like which style to use).

@vojtechjelinek Thanks for letting me know!

@Hudda Would you be able to change the issue description so that it's ready to be tagged with good-first-issue label? Asking you as a Linter team member, and also since you would have better idea here, and would understand what @vojtechjelinek is exactly suggesting. I think providing some idea about basic flow to solve this issue (with helpful links to wiki as needed) would be good. Also I believe it can serve as a very good start point for first time contributors. Thanks a lot!!

activenod commented 3 years ago

@vojtechjelinek @DubeySandeep Hi, I am new here and would like to contribute to this issue. I tried adding @typescript-eslint/quotes to .eslintrc file and ran python -m scripts.linters.pre_commit_linter --only-check-file-extensions js. I don't any issues, all checks were successfully passed. Am I doing anything wrong here?. Please guide me.

Hudda commented 3 years ago

@vojtechjelinek @DubeySandeep Hi, I am new here and would like to contribute to this issue. I tried adding @typescript-eslint/quotes to .eslintrc file and ran python -m scripts.linters.pre_commit_linter --only-check-file-extensions js. I don't any issues, all checks were successfully passed. Am I doing anything wrong here?. Please guide me.

@activenod This command will run script only on js files that you have changed. But you need to run the script on all js files in the codebase, which can be done by this command python -m scripts.linters.pre_commit_linter --only-check-file-extensions=js --path=. --verbose. If it takes too much time you can directly push the code to github and github actions will run these checks for you. I've updated the instructions in the issue description accordingly. Thanks!

activenod commented 3 years ago

@Hudda Please assign me @typescript-eslint/quotes

Hudda commented 3 years ago

@activenod I've assigned you. Thanks!

oppiabot[bot] commented 3 years ago

Hi @oppia/core-maintainers, this issue is not assigned to any project. Can you please update the same? Thanks!

huntermaxfield commented 3 years ago

@Hudda please assign me @typescript-eslint/dot-notation

Hudda commented 3 years ago

@huntermaxfield I've assigned you. Thanks!

mahendra1290 commented 3 years ago

@Hudda i want to work on this can you please guide me through this

Hudda commented 3 years ago

@mahendra1290 all the steps are listed in the issue description. Please take a look at that.

aakashraj01 commented 3 years ago

@Hudda Hi, I would like to work on this issue. This is my first open-source contribution so please assign me a file accordingly.

Lawful2002 commented 3 years ago

@Hudda, I would like to work on typescript-eslint/space-before-function-paren. Also I noticed that including typescript-eslint/keyword-spacing and @typescript-eslint/comma-spacing in the .eslintrc doesn't throw any errors.

Hudda commented 3 years ago

@Hudda, I would like to work on typescript-eslint/space-before-function-paren. Also I noticed that including typescript-eslint/keyword-spacing and @typescript-eslint/comma-spacing in the .eslintrc doesn't throw any errors.

@Lawful2002 I've assigned you to these checks.

SanjaySajuJacob commented 3 years ago

@Hudda I would like to work on @typescript-eslint/method-signature-style. I am new to opensource contribution and this is my first issue.

Hudda commented 3 years ago

@SanjaySajuJacob I've assigned you, you can start working on this one.

Lawful2002 commented 3 years ago

@Hudda the property space-before-function-paren already exists in the .eslintrc file:

"space-before-function-paren": [
      "error",
      {
        "anonymous": "never",
        "asyncArrow": "never",
        "named": "never"
      }
    ],

do I need to update it from never (default) to always ?

PranshuSrivastava commented 3 years ago

Hey @Hudda , could you assign me the @typescript-eslint/consistent-type-assertions rule?

Hudda commented 3 years ago

@Hudda the property space-before-function-paren already exists in the .eslintrc file:

"space-before-function-paren": [
      "error",
      {
        "anonymous": "never",
        "asyncArrow": "never",
        "named": "never"
      }
    ],

do I need to update it from never (default) to always ?

Sorry for the delay. Yes, you need to change it to always.

Hudda commented 3 years ago

Hey @Hudda , could you assign me the @typescript-eslint/consistent-type-assertions rule?

@PranshuSrivastava You are assigned.

vojtechjelinek commented 3 years ago

@Lawful2002 Hey, sorry about this, but actually I think @Hudda is wrong. What you need to do is to enable the @typescript-eslint/space-before-function-paren and disable the space-before-function-paren check. The rules for the @typescript-eslint/space-before-function-paren should stay the same as for space-before-function-paren.

Lawful2002 commented 3 years ago

@vojtechjelinek So I need to add

"space-before-function-paren": "off",
....
....
"@typescript-eslint/space-before-function-paren": [
      "error",
      {
        "anonymous": "never",
        "asyncArrow": "never",
        "named": "never"
      }
    ],
vojtechjelinek commented 3 years ago

@Lawful2002 Yes

ShivamJhaa commented 3 years ago

Hey @Hudda, I would like to work on @typescript-eslint/func-call-spacing if available.

Hudda commented 3 years ago

@ShivamJhaa I've assigned you!

IniobongUdom7 commented 3 years ago

Hello @Hudda, can you please assign me to work on @typeScript-eslint/promise-function-async?

Hudda commented 3 years ago

@IniobongUdom7 You are assigned.

nithinrdy commented 3 years ago

Hello @Hudda, may I work on @typescript-eslint/consistent-type-definitions?

Hudda commented 3 years ago

@nithinrdy I've assigned you.

nithinrdy commented 3 years ago

@Hudda , may I work on @typescript-eslint/member-delimiter-style?

Hudda commented 3 years ago

@nithinrdy you are assigned.

nithinrdy commented 3 years ago

@vojtechjelinek I am using the following configuration for the 'member-delimiter-style' rule:

image

I would like to know if these are appropriate. More specifically, the underlined part is what I'd like to know about. With it set to true: image

With it set to false: image

Which one should I be using?

vojtechjelinek commented 3 years ago

@vojtechjelinek I am using the following configuration for the 'member-delimiter-style' rule:

image

I would like to know if these are appropriate. More specifically, the underlined part is what I'd like to know about. With it set to true: image

With it set to false: image

Which one should I be using?

I think false is better, also it seems to be used in the default config . I think we can just use the default config.

IamEzio commented 2 years ago

@Hudda @vojtechjelinek Can @typescript-eslint/lines-between-class-members be assigned to me?

khareyash05 commented 2 years ago

@Hudda Pls assign me @typescript-eslint/no-for-in-array

vojtechjelinek commented 2 years ago

@khareyash05 Done.

pathange-s commented 2 years ago

@Hudda Please assign me @typescript-eslint/explicit-function-return-type

Hudda commented 2 years ago

@pathange-s You are assigned.

ParmeetChawla25 commented 2 years ago

Hi @Hudda can you assign me @typescript-eslint/await-thenable

Hudda commented 2 years ago

@ParmeetChawla25 I've assigned you.

DubeyRicha commented 2 years ago

@vojtechjelinek can you please assign me @typescript-eslint/no-unnecessary-condition?

vojtechjelinek commented 2 years ago

@DubeyRicha Done.

BlakeHan01 commented 2 years ago

@vojtechjelinek Can you please assign me @typescript-eslint/no-invalid-void-type?

vojtechjelinek commented 2 years ago

@BlakeHan01 Done.

BlakeHan01 commented 2 years ago

@vojtechjelinek I have fixed all errors but I couldn't fix this one in the second image on line 178. I tried changing void to undefined but it wouldn't let me. Could you provide some help with it? Thanks.

image image
vojtechjelinek commented 2 years ago

@BlakeHan01 Can you put just FeedbackThread[] there?

BlakeHan01 commented 2 years ago

@vojtechjelinek Unfortunately I get the same error. Do you still think it's possible to fix this error without changing other parts of the codebase?

image
vojtechjelinek commented 2 years ago

@BlakeHan01 Maybe not, can you try to change the code so that void is not returned?

BlakeHan01 commented 2 years ago

@vojtechjelinek I have tried changing the code in this file and in the file where FeedbackThread[] is defined. However, the return type has to be void | FeedbackThread[]. Could you please remove this rule as it probably can't be fixed without changing a lot of the codebase which might not be so beginner-friendly?