-
-
Notifications
You must be signed in to change notification settings - Fork 887
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
Fixed some TypeScript Lint warnings. #1390
Fixed some TypeScript Lint warnings. #1390
Conversation
Our Pull Request Approval ProcessWe have these basic policies to make the approval process smoother for our volunteer team. Testing Your CodePlease make sure your code passes all tests. Our test code coverage system will fail if these conditions occur:
The process helps maintain the overall reliability of the code base and is a prerequisite for getting your PR approved. Assigned reviewers regularly review the PR queue and tend to focus on PRs that are passing. ReviewersWhen your PR has been assigned reviewers contact them to get your code reviewed and approved via:
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
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.
Congratulations on making your first PR! 🎊 If you haven't already, check out our Contributing Guidelines and PR Reporting Guidelines to ensure that you are following our guidelines for contributing and creating PR.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #1390 +/- ##
===========================================
- Coverage 98.17% 97.62% -0.56%
===========================================
Files 184 214 +30
Lines 10767 12950 +2183
Branches 835 1052 +217
===========================================
+ Hits 10571 12643 +2072
- Misses 186 292 +106
- Partials 10 15 +5 ☔ View full report in Codecov by Sentry. |
Hey @tasneemkoushar, |
Hey @palisadoes can you review my PR please. Thanks. |
@tasneemkoushar Please review |
@tasneemkoushar Please review |
This pull request did not get any activity in the past 10 days and will be closed in 180 days if no update occurs. Please verify it has no conflicts with the develop branch and rebase if needed. Mention it now if you need help or give permission to other people to finish your work. |
@git-init-priyanshu Shouldn't you also be editing the |
@@ -48,7 +48,7 @@ export const createDirectChat: MutationResolvers["createDirectChat"] = async ( | |||
_id: args.data.organizationId, | |||
}).lean(); | |||
|
|||
await cacheOrganizations([organization!]); | |||
if (organization) await cacheOrganizations([organization]); |
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.
You have added this if (organization)
check in all of the caching statements to remove the use of the !
operator. Still, the same is unnecessary as it is guaranteed to exist, and thus the use of the !
operator. This change adds unnecessary verbosity to the code.
We should revert this change or find a more elegant way to deal with the same. What do you think @kb-0311?
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.
Ok got it. I think I need to find a better way to remove the !
operator
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.
Hey, for this can we do something like this?: we can check if we are getting id
from the args
and if not we can throw an error. So this will make sure that that the ids we are passing to the findOrganizationsInCache
function will never be null.
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.
Please also add all these rules to the linting config files so that husky and the workflows catch them.
Please fix the conflicting file |
@git-init-priyanshu can you resolve the conflicts? |
@git-init-priyanshu Please fix the conflicting files. |
I apologize for the inactivity. I have semester exams starting next week, so I won't have the time to work on this. Could you please grant me more time? Thank you. |
@git-init-priyanshu you can do it after your exam. FYI some linting issues have been fixed by @parthiv360 . You can check that here in this PR. Please resolve conflicting files and try to fix linting issues which aren't covered in PR #1412 once your exam finishes. |
Yeah, sure. |
@git-init-priyanshu Please fix the conflicting files. |
@git-init-priyanshu any update on this PR |
My exams just finished today, so I will be working on this now. |
@tasneemkoushar I have made the changes. Can you check now. |
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.
Needs minor changes.
@@ -190,7 +191,8 @@ describe("utilities -> imageAlreadyInDbCheck", () => { | |||
|
|||
try { | |||
await imageAlreadyInDbCheck(null, testNewImagePath); | |||
} catch (error: any) { | |||
} catch (error: unknown) { | |||
if (!(error instanceof ApplicationError)) return; |
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.
Make the test fail in such a case.
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.
Make the test fail in such a case.
if (!(error instanceof Error)) throw new Error;
Sorry for asking but will it work?
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.
This is not idiomatic.
Use Vitest error assertions instead.
@@ -32,7 +33,8 @@ describe("src -> utilities -> encodedImageStorage -> uploadEncodedImage", () => | |||
"NAAAAKElEQVQ4jWNgYGD4Twzu6FhFFGYYNXDUwGFpIAk2E4dHDRw1cDgaCAASFOffhEIO" + | |||
"3gAAAABJRU5ErkJggg=="; | |||
await uploadEncodedImage(img, null); | |||
} catch (error: any) { | |||
} catch (error: unknown) { | |||
if (!(error instanceof ApplicationError)) return; |
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.
Make the test fail here instead of returning.
tests/utilities/adminCheck.spec.ts
Outdated
@@ -50,7 +50,8 @@ describe("utilities -> adminCheck", () => { | |||
testUser?._id, | |||
testOrganization ?? ({} as InterfaceOrganization) | |||
); | |||
} catch (error: any) { | |||
} catch (error: unknown) { | |||
if (!(error instanceof Error)) return; |
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.
Ditto.
Hey @EshaanAgg can you please check now. |
Please fix the failing workflows. |
can you check now |
@git-init-priyanshu The tests are still failing. |
…a-api-clone into git-init-priyanshu
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.
The PR has the right spirit but needs work. Instead of trying to fix the lining warnings, please try to understand the intent behind using !
or ?
and then write the resolvers accordingly. TypeScript and lining rules should help to enforce these patterns, not bother.
@@ -150,7 +150,7 @@ export const createPost: MutationResolvers["createPost"] = async ( | |||
} | |||
); | |||
|
|||
await cacheOrganizations([updatedOrganizaiton!]); | |||
if (updatedOrganizaiton) await cacheOrganizations([updatedOrganizaiton]); |
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.
In all such cases we are assured that the updatedOrganization
would be not null due to the business logic. Thus this conditional is useless.
@@ -55,7 +55,7 @@ export const createPost: MutationResolvers["createPost"] = async ( | |||
_id: args.data.organizationId, | |||
}).lean(); | |||
|
|||
await cacheOrganizations([organization!]); | |||
if (organization !== null) await cacheOrganizations([organization]); |
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.
Ditto.
@@ -42,7 +42,7 @@ export const removeMember: MutationResolvers["removeMember"] = async ( | |||
_id: args.data.organizationId, | |||
}).lean(); | |||
|
|||
await cacheOrganizations([organization!]); | |||
if (organization !== null) await cacheOrganizations([organization]); |
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.
Ditto
updatedUser!.image = updatedUser?.image | ||
? `${context.apiRootUrl}${updatedUser?.image}` | ||
: null; | ||
if (updatedUser) |
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.
Ditto.
…a-api-clone into git-init-priyanshu
Please close this PR and the issue if the issue above is a more suitable solution. |
I totally agree with you, sir. I was also losing motivation to work on this PR. I will review my methodology and make sure to work on this. If you have any specific suggestions or insights, I would welcome them to ensure that I can improve. |
|
What kind of change does this PR introduce?
This pull request addresses some of the TypeScript lint warnings.
Issue Number:
#1201
Fixes #1201
Did you add tests for your changes?
No. I have tested it against the existing test cases.
Snapshots/Videos:
N/A
If relevant, did you update the documentation?
No
Summary
This PR fixes the following lint warnings:
Does this PR introduce a breaking change?
No
Have you read the contributing guide?
Yes