chore(fe): unify TaggedObject type usage + corresponding refactor #31969
Conversation
Signed-off-by: hainenber <dotronghai96@gmail.com>
hainenber
requested review from
sadpandajoe,
geido,
eschutho,
rusackas and
betodealmeida
as code owners
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Fix Detected |
|---|---|---|
 |
Inconsistent tagIds parameter type ▹ view | ✅ |
Need a new review? Comment
/korbit-reviewon this PR and I'll review your latest changes.Korbit Guide: Usage and Customization
Interacting with Korbit
- You can manually ask Korbit to review your PR using the
/korbit-reviewcommand in a comment at the root of your PR.- You can ask Korbit to generate a new PR description using the
/korbit-generate-pr-descriptioncommand in any comment on your PR.- Too many Korbit comments? I can resolve all my comment threads if you use the
/korbit-resolvecommand in any comment on your PR.- Chat with Korbit on issues we post by tagging @korbit-ai in your reply.
- Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.
Customizing Korbit
- Check out our docs on how you can make Korbit work best for you and your team.
- Customize Korbit for your organization through the Korbit Console.
Feedback and Support
| tagIds = [], | ||
| types, | ||
| }: { tagIds: number[] | undefined; types: string | null }, | ||
| { tagIds = [], types }: { tagIds: number[] | string; types: string | null }, |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
@korbit-ai The fetchObject function in this file also have string type for tagsId so the old behavior is OK
There was a problem hiding this comment.
@hainenber Thank you for pointing that out. It makes sense to maintain the original structure if other functions are using the same type. However, for long-term consistency and error reduction, we could consider standardizing the data types used in API parameters across functions. What do you think about gradually refactoring other functions to only accept array of numbers for tagIds as well?
There was a problem hiding this comment.
@korbit-ai Sure, can we leave it for another PR?
There was a problem hiding this comment.
@hainenber I haven't looked at the tagID format(s) - does this feedback make sense?
There was a problem hiding this comment.
Okay, turned out the fetchObject function I referred above is not used anywhere 🤦 . No point in using it to defend keeping old behavior.
Okay, Korbot makes sense here. However, since tag?.id in the old code will assume undefined value, I'll change the parameter to accommodate the old behavior as well.
There was a problem hiding this comment.
update in HEAD :D
…function Signed-off-by: hainenber <dotronghai96@gmail.com>
| @@ -152,12 +140,16 @@ function AllEntities() { | |||
| return; | |||
| } | |||
| fetchObjectsByTagIds( | |||
| { tagIds: [tag?.id] || '', types: null }, | |||
There was a problem hiding this comment.
This is always evaluated to truthy so it'll always give out (undefined | number)[]
chore(fe): unify TaggedObject type usage + corresponding refactor
SUMMARY
Part of rectification of existing type errors before TypeScript v5 migration.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before
After
TESTING INSTRUCTIONS
All CI checks should pass
ADDITIONAL INFORMATION