[Discovery] Feasibility of applying amnesty on the existing eslint warnings

[UsamaSadiq]

Description

All the pylint violations in edx-platform have been suppressed using lint-amnesty. Now the next target is to apply amnesty on the eslint warnings and remove its threshold.

Acceptance Criteria

• Do a discovery on the possible methods/impact of applying lint-amnesty on the eslint warnings. There seems to be a comment syntax for this purpose but we need to find out if we can quickly automate adding those comments through lint-amnesty.
• Create a plan with the finalised tool and create follow up issues to complete this refactoring.

Initial Research on the Issue

• esplint: A tool to record existing violations in a separate json file and only show new warnings when running in CI. The details to use this can be found in this article. Pros: Using this tool can decrease the time required to disable all the warnings across repo in multiple PRs. Cons: It’ll not help in identifying the failing lines in the code since it won’t highlight anything in the code itself and will only keep a record separately.

• suppress-eslint-errors: A tool to add the disable message on all the existing eslint violations in the code. Pros: Using this tool can help us in identifying disabled warnings in the codebase which will be helpful later on in resolving these warnings. Cons: This will need to be applied through the automated job and multiple PRs across edx-platform will be created to cover 5507 (current no. of eslint warnings). To use this tool, we’ll first need to update the eslint rules to identify all the warnings as errors since this tool can only work on disabling the errors and not warnings.

[arbrandes]

@Mashal-m, my instincts says to go with suppress-eslint-errors, but... wow, that's going to be a lot of PRs. Can we at least have one single refactor: PR per repository?

@adamstankiewicz, I'm curious what your take would be on this one.

[arbrandes]

@abdullahwaheed, can we get a status update, here? Thanks!

[abdullahwaheed]

@arbrandes we were focusing on removing legacy code from platform first so we can focus on remaining warnings

[Mashal-m]

@UsamaSadiq after merging this PR Migrate eslint-config-edx the eslint report is zero.

[UsamaSadiq]

The warning report shouldn't be affected this much with package migration. To investigate the warning report accuracy, I can think of following two methods:

• Compare the configs of both old and new eslint-config-edx and @edx/eslint-config to see if we need to explicitly enable some of the warnings to be captured in the new package.
• Create a test PR manually introducing an eslint warning which should be detected by the package and verify that warning report includes the introduced warning. If this works, then we can go with the new package as it is since it'll be capturing the warnings as expected.

FYI @abdullahwaheed @Mashal-m

[arbrandes]

Can I get a status update here? Thanks!

[Syed-Ali-Abbas-Zaidi]

Hi @arbrandes, After conducting RnD on this issue, we have discovered that eslint-config-edx was not migrated correctly in that particular pull request (PR). Consequently, we have divided this task into three steps:

1. Rectify the migration issue of eslint-config-edx.
2. Resolve all eslint issues that can be automatically fixed.
3. Apply amnesty to all existing issues that cannot be automatically fixed.

We have completed steps 1 and 2. Step 3 is currently in progress, and the PRs associated with it are under review. Once the reviews are completed, we will be ready to deploy them as well.

[arbrandes]

@Syed-Ali-Abbas-Zaidi, just checking: are we still going to pursue this?

[abdullahwaheed]

@Syed-Ali-Abbas-Zaidi, just checking: are we still going to pursue this?

@arbrandes we will resume this after edx-platform frontend upgrade to latest versions