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: Updated eslint configuration #2851

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

Conversation

jsumners-nr
Copy link
Contributor

@jsumners-nr jsumners-nr commented Dec 23, 2024

This PR resolve #2852.

This PR updates the eslint configuration, and plugins, to the latest. I discovered that our shared configuration has not kept up-to-date on the packages that it uses. In particular:

  1. eslint released v9 that radically changes (and simplifies) how configuration is written and parsed.
  2. eslint-plugin-node has been out-of-maintenance for a long time, replaced by eslint-plugin-n

The result was a difficult to update shared configuration package. Now that this PR is ready, I could go back and update the shared package to eslint@9 standards, but I'm not convinced the shared package is worth the maintenance cost. Most of the configuration is specific to the repository. I am quite certain that most of the packages we maintain can get by with the simple neostandard configuration.

Having been out-of-date for so long, our code base has developed many "errors" in regard to linting. The sonarjs plugin saw a lot of additions that now trigger errors. So there are many files touched in this PR. I took the approach of overriding the majority of rules for the tests, but using inline comments in the actual library code. I think our tests can be more lenient, and that we can investigate on a case-by-case basis all of the manual overrides.

I think we should wait for the next branch to be merged to main, at which point I will rebase and update this branch, before we merge. But I'm open to merging this first if the team thinks it will not cause too much strife in keeping next synchronized.

@jsumners-nr jsumners-nr force-pushed the neostandard branch 5 times, most recently from 8e504e9 to 6c63650 Compare December 23, 2024 18:57
Copy link

codecov bot commented Dec 24, 2024

Codecov Report

Attention: Patch coverage is 98.70130% with 3 lines in your changes missing coverage. Please review.

Project coverage is 97.22%. Comparing base (30a6de0) to head (af3771d).

Files with missing lines Patch % Lines
lib/instrumentation/when/index.js 66.66% 1 Missing ⚠️
lib/transaction/index.js 87.50% 1 Missing ⚠️
lib/transaction/name-state.js 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2851      +/-   ##
==========================================
+ Coverage   88.74%   97.22%   +8.47%     
==========================================
  Files         294      295       +1     
  Lines       46447    46466      +19     
==========================================
+ Hits        41221    45175    +3954     
+ Misses       5226     1291    -3935     
Flag Coverage Δ
integration-tests-cjs-18.x 73.14% <69.81%> (-0.02%) ⬇️
integration-tests-cjs-20.x 73.14% <69.81%> (-0.02%) ⬇️
integration-tests-cjs-22.x 73.18% <69.81%> (-0.02%) ⬇️
integration-tests-esm-18.x 49.90% <42.98%> (-0.02%) ⬇️
integration-tests-esm-20.x 49.91% <42.98%> (-0.02%) ⬇️
integration-tests-esm-22.x 49.96% <42.98%> (-0.02%) ⬇️
unit-tests-18.x 88.91% <93.44%> (?)
unit-tests-20.x 88.90% <93.44%> (?)
unit-tests-22.x 88.92% <93.44%> (?)
versioned-tests-18.x 79.02% <75.21%> (-0.18%) ⬇️
versioned-tests-20.x 79.03% <75.21%> (-0.18%) ⬇️
versioned-tests-22.x 79.04% <75.21%> (-0.18%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@@ -46,7 +46,7 @@ test('should correctly convert bools when false', async () => {
})

test('should correctly convert integers', async () => {
const intValue = 9999999999999999
const intValue = 999999999999999
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one generated an error about the value overflowing. I don't think we were trying to test against an overflowed value, so I just shortened it to fit.

@jsumners-nr jsumners-nr marked this pull request as ready for review December 24, 2024 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Needs PR Review
Development

Successfully merging this pull request may close these issues.

Update eslint configuration
1 participant