-
Notifications
You must be signed in to change notification settings - Fork 146
fix: linter warnings and CI failure #1218
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
Conversation
✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1218 +/- ##
==========================================
- Coverage 84.11% 84.06% -0.05%
==========================================
Files 68 68
Lines 2958 2944 -14
Branches 373 373
==========================================
- Hits 2488 2475 -13
+ Misses 410 409 -1
Partials 60 60 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@kriswest Note that the Promise wrappers are not entirely gone, apparently these are required to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved - easy to review as I've just implemented the same changes and was about to raise the PR. I'd used promise chains in a couple of places and slightly more error handling, but this also looks fine to me.
Promisify will do approximately the same thing so I doubt theres much value in changing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
theres one unreachable line, I'm surprised the linter isn't complaining about it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Signed-off-by: Kris West <kristopher.west@natwest.com>
Probably should yes, but in its own PR (in case we've got unreachable code elsewhere that we can clear up at the same time)
Sent from Outlook for Android<https://aka.ms/AAb9ysg>
________________________________
From: Juan Escalada ***@***.***>
Sent: Wednesday, September 24, 2025 3:52:41 PM
To: finos/git-proxy ***@***.***>
Cc: West, Kristopher (Architecture and Engineering, Digital X) ***@***.***>; Mention ***@***.***>
Subject: Re: [finos/git-proxy] fix: linter warnings and CI failure (PR #1218)
*********************************************
"This is an external email. Do you know who has sent it? Can you be sure that any links and attachments contained within it are safe? If in any doubt, use the Phishing Reporter Button in your Outlook client or forward the email to ~ I've Been Phished"
*********************************************
@jescalada commented on this pull request.
________________________________
In src/db/file/repo.ts<#1218 (comment)>:
@@ -134,21 +133,21 @@ export const addUserCanPush = async (_id: string, user: string): Promise<void> =
export const addUserCanAuthorise = async (_id: string, user: string): Promise<void> => {
user = user.toLowerCase();
- return new Promise(async (resolve, reject) => {
- const repo = await getRepoById(_id);
- if (!repo) {
- reject(new Error('Repo not found'));
- return;
- }
+ const repo = await getRepoById(_id);
+ if (!repo) {
+ throw new Error('Repo not found');
+ return;
This can be fixed by adding 'no-unreachable': 'error' to our current ESLint config (just tested and it works). Should we do that? 🤔
—
Reply to this email directly, view it on GitHub<#1218 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAM7PBFNUSA2PBEBIIBOLVL3UKV3TAVCNFSM6AAAAACHMC3SUCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTENRTGMYTQMBXGY>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
National Westminster Bank plc. Registered in England No. 929027. Registered Office: 250 Bishopsgate, London EC2M 4AA. National Westminster Bank plc is authorised by the Prudential Regulation Authority, and regulated by the Financial Conduct Authority and the Prudential Regulation Authority.
The Royal Bank of Scotland plc. Registered in Scotland No. 83026. Registered Office: 36 St Andrew Square, Edinburgh EH2 2YB. The Royal Bank of Scotland plc is authorised by the Prudential Regulation Authority, and regulated by the Financial Conduct Authority and the Prudential Regulation Authority.
The Royal Bank of Scotland plc and National Westminster Bank plc are authorised to act as agent for each other.
The Royal Bank of Scotland plc and National Westminster Bank plc are UK chartered banks and are not chartered or licensed as banks by the United States or any individual state.
This e-mail message is confidential and for use by the addressee only. If the message is received by anyone other than the addressee, please return the message to the sender by replying to it and then delete the message from your computer. Internet e-mails are not necessarily secure. The Royal Bank of Scotland plc, National Westminster Bank plc or any affiliated entity (“NatWest” or “us”) does not accept responsibility for changes made to this message after it was sent. NatWest may monitor e-mails for business and operational purposes. By replying to this message you give your consent to the monitoring of your e-mail communications with us. Whilst all reasonable care has been taken to avoid the transmission of viruses, it is the responsibility of the recipient to ensure that the onward transmission, opening or use of this message and any attachments will not adversely affect its systems or data. No responsibility is accepted by NatWest in this regard and the recipient should carry out such virus and other checks as it considers appropriate.
|
Fixes #1212 (linter warnings/CI failures that popped up after #955).
Notably, some of the errors were due to wrapping promises for some of the database functions (this is only necessary when calling neDB functions directly, because they contain callbacks which can't be intercepted otherwise).