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

Resolve eslinting errors #2654

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ADI-ROXX
Copy link
Contributor

@ADI-ROXX ADI-ROXX commented Feb 5, 2025

Which problem is this PR solving?

Resolves #2653

Description of the changes

How was this change tested?

  • npm run lint and
  • npm run test

Checklist

renovate-bot and others added 2 commits February 5, 2025 11:50
Signed-off-by: cs-308-2023 <[email protected]>
@ADI-ROXX ADI-ROXX requested a review from a team as a code owner February 5, 2025 16:26
@ADI-ROXX ADI-ROXX requested review from pavolloffay and removed request for a team February 5, 2025 16:26
@ADI-ROXX ADI-ROXX changed the title Resolves eslinting errors Resolve eslinting errors Feb 5, 2025
@ADI-ROXX
Copy link
Contributor Author

ADI-ROXX commented Feb 5, 2025

npm ci is failing. Working on it

Signed-off-by: cs-308-2023 <[email protected]>
Signed-off-by: cs-308-2023 <[email protected]>
Copy link

codecov bot commented Feb 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.62%. Comparing base (e3776db) to head (e740f1c).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2654   +/-   ##
=======================================
  Coverage   96.62%   96.62%           
=======================================
  Files         255      255           
  Lines        7726     7726           
  Branches     2011     1939   -72     
=======================================
  Hits         7465     7465           
  Misses        261      261           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: cs-308-2023 <[email protected]>
@ADI-ROXX ADI-ROXX force-pushed the renovate/major-typescript-eslint-monorepo branch from 4f0a259 to 9cf74f3 Compare February 6, 2025 16:46
@ADI-ROXX
Copy link
Contributor Author

ADI-ROXX commented Feb 6, 2025

@yurishkuro I am getting the following error:

image

I am thinking to remove all the shown dependencies. Shall I go ahead with it?

@yurishkuro
Copy link
Member

I am thinking to remove all the shown dependencies. Shall I go ahead with it?

Something tells me you broke something in the build if these are reported as unused. Maybe some renamed files are no longer being considered by the depcheck.

Overall I am not comfortable with the level of the changes here. I would like to see written motivation (e.g. a separate issue) for different/independent changes, and have them in isolated PRs where they can be independently validated, not bundled together along with the upgrade.

@ADI-ROXX
Copy link
Contributor Author

ADI-ROXX commented Feb 7, 2025

Maybe some renamed files are no longer being considered by the depcheck.

Okay. I'll revert the changes and try to debug.

I would like to see written motivation (e.g. a separate issue) for different/independent changes, and have them in isolated PRs where they can be independently validated, not bundled together along with the upgrade.

In this PR, there are many types of changes:

  1. Changes related to ESLint.
  2. Changes related to solving npm ci after npm run lint runs perfectly.
  3. Changes related to solving npm run depcheck after step 2

So, as far as I can see, these 3 are the isolated pr's that have to be added.
However, I would like top point it out that the first two pr's will fail a lot of tests, only the third pr will succeed in all.

independently validated

Change 1 cannot be independently validated because npm run lint runs after npm ci and npm run depcheck. The best that can be done is to run on local and add the screenshot of the same before merging.

@yurishkuro
Copy link
Member

I was not referring to compile phases as differences but to the actual code changes. For instance, renaming something to .mjs is a certain type of change that has specific purpose and can be done without any eslint upgrades. Same for replacing require with imports. Just do them incrementally while maintaining green CI.

@ADI-ROXX
Copy link
Contributor Author

ADI-ROXX commented Feb 7, 2025

Just do them incrementally while maintaining green CI.

Got it. I will create another pr then. It will have the commits in incremental manner. I will try to document the changes.

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.

Upgrade typescript-eslint
3 participants