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: update Promise.reject()(s) across codebase to throw Error("message"), instead of regular string #5144

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

jimezesinachi
Copy link

@jimezesinachi jimezesinachi commented Jan 21, 2024

Changes
This change removes some code smell in the database. According to this rule (https://sonarcloud.io/organizations/jellyfin/rules?open=javascript%3AS6671&rule_key=javascript%3AS6671) on the project's SonarCloud, any Promise.reject call in the repository that currently returns a string, should be updated to return a throw Error('message'), instead. This PR includes changes to different parts of the codebase with this issue.

Issues

Signed-off-by: Jim Ezesinachi <ezesinachijim@gmail.com>
@jimezesinachi jimezesinachi requested a review from a team as a code owner January 21, 2024 00:27
@jimezesinachi jimezesinachi changed the title chore: update Promise.reject(s) across codebase to throw Error("message"), instead of regular string chore: update Promise.reject()(s) across codebase to throw Error("message"), instead of regular string Jan 21, 2024
Signed-off-by: Jim Ezesinachi <ezesinachijim@gmail.com>
@jimezesinachi
Copy link
Author

I've made two of these changes so far. I will proceed adding more to this PR once I get a response from a maintainer that this change is still useful. Thank you!

@grafixeyehero
Copy link
Member

LGTM

Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@jellyfin-bot jellyfin-bot added the merge conflict Conflicts prevent merging label Feb 23, 2024
@jellyfin-bot
Copy link
Collaborator

This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged.

@thornbill
Copy link
Member

Hey @jimezesinachi! This looks good! Now that 10.9 is finally out the door it would be good to get these fixed. Thanks!

@thornbill thornbill added the cleanup Cleanup of legacy code or code smells label May 18, 2024
@thornbill thornbill added this to the v10.10.0 milestone May 18, 2024
@jimezesinachi
Copy link
Author

Excellent! Do you want to merge in this one no, or should I add a couple other changes for the same rule?

@thornbill
Copy link
Member

Let's just add the others here 👍

@jimezesinachi
Copy link
Author

Let's just add the others here 👍

Excellent. Will work on this on the weekend 👍🏾

…hore/refactor-promise-rejections-to-throw-error
@jellyfin-bot jellyfin-bot removed the merge conflict Conflicts prevent merging label Sep 7, 2024
@jimezesinachi
Copy link
Author

Updated the branch with master... good to merge now

@jimezesinachi
Copy link
Author

Note: from the wording of the rule (here: https://sonarcloud.io/organizations/jellyfin/rules?open=javascript%3AS6671&rule_key=javascript%3AS6671), my understanding is that it specifically applies any reject's in the codebase that are currently being passed just a string (i.e reject("ComponentName error!")). There are currently no other ones that match this interpretation, as the other ones are either already returning error (i.e reject(new Error("ComponentName error!"))), or just rejecting without any value passed (i.e (reject()).

LMK what you think @thornbill @grafixeyehero @tomislav

@grafixeyehero
Copy link
Member

Note: from the wording of the rule (here: https://sonarcloud.io/organizations/jellyfin/rules?open=javascript%3AS6671&rule_key=javascript%3AS6671), my understanding is that it specifically applies any reject's in the codebase that are currently being passed just a string (i.e reject("ComponentName error!")). There are currently no other ones that match this interpretation, as the other ones are either already returning error (i.e reject(new Error("ComponentName error!"))), or just rejecting without any value passed (i.e (reject()).

LMK what you think @thornbill @grafixeyehero @tomislav

First, we Thanks for your contribution. To ensure consistency in our error handling, we should refactor the rejections that currently use strings or have no value to use Error objects instead.

@jimezesinachi
Copy link
Author

Note: from the wording of the rule (here: https://sonarcloud.io/organizations/jellyfin/rules?open=javascript%3AS6671&rule_key=javascript%3AS6671), my understanding is that it specifically applies any reject's in the codebase that are currently being passed just a string (i.e reject("ComponentName error!")). There are currently no other ones that match this interpretation, as the other ones are either already returning error (i.e reject(new Error("ComponentName error!"))), or just rejecting without any value passed (i.e (reject()).

LMK what you think @thornbill @grafixeyehero @tomislav

First, we Thanks for your contribution. To ensure consistency in our error handling, we should refactor the rejections that currently use strings or have no value to use Error objects instead.

Understood. Just one more thing: for the ones that have no value, we can just pass an Empty error object (i.e reject new Error()), yes?

@grafixeyehero
Copy link
Member

You can give it a value based on what action was rejected

@jimezesinachi
Copy link
Author

You can give it a value based on what action was rejected

Understood. On it now 👍🏾

Copy link

@jellyfin-bot
Copy link
Collaborator

jellyfin-bot commented Sep 21, 2024

Cloudflare Pages deployment

Latest commit 3aaaeef
Status ✅ Deployed!
Preview URL https://6360c746.jellyfin-web.pages.dev
Type 🔀 Preview

View build logs

@thornbill thornbill removed this from the v10.10.0 milestone Oct 15, 2024
@thornbill
Copy link
Member

@jimezesinachi do you still plan to correct the other instances of this? Thanks!

@jimezesinachi
Copy link
Author

@jimezesinachi do you still plan to correct the other instances of this? Thanks!

Affirmative... I'm still away from work, but this is on my agenda when I get back to work

@jimezesinachi
Copy link
Author

Back in today... working on this now

Signed-off-by: Jim Ezesinachi <ezesinachijim@gmail.com>
Signed-off-by: Jim Ezesinachi <ezesinachijim@gmail.com>
@jimezesinachi
Copy link
Author

jimezesinachi commented Jan 20, 2025

Updates are up and Actions check jobs completed

@jimezesinachi
Copy link
Author

@thornbill quick review on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Cleanup of legacy code or code smells
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants