Skip to content

Conversation

06kellyjac
Copy link
Contributor

Replaces #890
Replaces #893

Raises some warnings but fixing them isn't possible without formatting many files.
I'll do that in another PR.

Copy link

netlify bot commented Mar 25, 2025

Deploy Preview for endearing-brigadeiros-63f9d0 canceled.

Name Link
🔨 Latest commit 97ef66b
🔍 Latest deploy log https://app.netlify.com/projects/endearing-brigadeiros-63f9d0/deploys/68d163d9f0cd8f00088b9791

Copy link

netlify bot commented Mar 25, 2025

Deploy Preview for endearing-brigadeiros-63f9d0 canceled.

Name Link
🔨 Latest commit bc4090c
🔍 Latest deploy log https://app.netlify.com/sites/endearing-brigadeiros-63f9d0/deploys/67e2df746363de0008ae0826

Copy link

codecov bot commented Mar 25, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.87%. Comparing base (169e0f3) to head (97ef66b).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #955   +/-   ##
=======================================
  Coverage   83.87%   83.87%           
=======================================
  Files          68       68           
  Lines        2908     2908           
  Branches      367      367           
=======================================
  Hits         2439     2439           
  Misses        409      409           
  Partials       60       60           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@06kellyjac
Copy link
Contributor Author

Ran into a couple roadblocks, holding off the styling fixes for now as the PR would be mega
@JamieSlome

@sam-holmes2
Copy link
Contributor

@06kellyjac let me know if you need a review on this once conflicts have been resolved

@kriswest
Copy link
Contributor

kriswest commented Sep 8, 2025

@06kellyjac if you could rebase this I think we should get it merged

@06kellyjac 06kellyjac force-pushed the eslint_v9 branch 2 times, most recently from 3b9ef07 to 8de0f3d Compare September 9, 2025 15:46
@06kellyjac
Copy link
Contributor Author

λ npm run lint

> @finos/git-proxy@2.0.0-rc.2 lint
> eslint


/somedir/git-proxy/cypress/e2e/autoApproved.cy.js
  66:5  warning  It is unsafe to chain further commands that rely on the subject after this command. It is best to split the chain, chaining again from `cy.` in a next command line  cypress/unsafe-to-chain-command

/somedir/git-proxy/cypress/e2e/repo.cy.js
  126:7  warning  It is unsafe to chain further commands that rely on the subject after this command. It is best to split the chain, chaining again from `cy.` in a next command line  cypress/unsafe-to-chain-command

/somedir/git-proxy/src/db/file/repo.ts
  108:22  warning  Promise executor functions should not be async  no-async-promise-executor
  136:22  warning  Promise executor functions should not be async  no-async-promise-executor
  165:22  warning  Promise executor functions should not be async  no-async-promise-executor
  189:22  warning  Promise executor functions should not be async  no-async-promise-executor

/somedir/git-proxy/src/db/index.ts
  103:31  warning  Promise executor functions should not be async  no-async-promise-executor
  119:22  warning  Promise executor functions should not be async  no-async-promise-executor
  139:22  warning  Promise executor functions should not be async  no-async-promise-executor

/somedir/git-proxy/test/chain.test.js
  61:5  warning  'mockPushProcessors' is defined but never used. Allowed unused vars must match /^_/u  @typescript-eslint/no-unused-vars

/somedir/git-proxy/test/testProxy.test.js
  267:39  warning  'callback' is defined but never used. Allowed unused args must match /^_/u  @typescript-eslint/no-unused-vars

✖ 11 problems (0 errors, 11 warnings)

Linting results should also show in CI.

Some of the comment ignores weren't relevant anymore after prettier fixes. And if they were, rules that prettier handles will be ignored by eslint now to prevent fighting.

Some of the removed ignores don't apply anymore. Some like no-invalid-this aren't working quite right currently but as we're ignoring it that's fine. It'll be caught with our move to TS.

@kriswest
Copy link
Contributor

@06kellyjac this looks good. However I note we get a few linting errors I'm wondering about:

export const isUserPushAllowed = async (url: string, user: string) => {
user = user.toLowerCase();
return new Promise(async (resolve) => {

Check warning on line 103 in src/db/index.ts

GitHub Actions / Linting

Promise executor functions should not be async

const repo = await getRepoByUrl(url);
if (!repo) {
resolve(false);
};

Should we be switching those to use promise chaining or disable that rule? Thinking about that example, the await should probably be wrapped in a try/catch as an error on the DB call might not cause isUserPushAllowed to reject... I'll a raise a separate PR to correct those if you concur.

@06kellyjac
Copy link
Contributor Author

They're raising problems we should be resolving so I'm inclined to keep them on & once resolved remove the item from the config to default back to "error" so we don't get any more cases

Signed-off-by: Kris West <kristopher.west@natwest.com>
Copy link
Contributor

@kriswest kriswest left a comment

Choose a reason for hiding this comment

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

LGTM

I've raised #1212 to resolve some of the check annotations reported.

@06kellyjac 06kellyjac merged commit 640bd00 into finos:main Sep 23, 2025
14 checks passed
@06kellyjac 06kellyjac deleted the eslint_v9 branch September 23, 2025 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants