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 (fix):removed shorthand type conversions #12457

Closed
wants to merge 0 commits into from
Closed

Chore (fix):removed shorthand type conversions #12457

wants to merge 0 commits into from

Conversation

Biki-das
Copy link
Contributor

In JavaScript, there are a lot of different ways to convert value types. Some of them might be hard to read and understand. specially if we try to use shorthand type conversion .

@@ -20,7 +20,7 @@ assert(process.argv[3], 'Pass the number of iterations');

const sleep = ms => new Promise(resolve => setTimeout(resolve, ms));
const method = process.argv[2];
const calls = +process.argv[3];
const calls = Number(process.argv[3]);
Copy link

@Smrtnyk Smrtnyk Feb 22, 2022

Choose a reason for hiding this comment

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

keep in mind that Number, Boolean and String can be overriden with something faulty, so +'string', !! and "" + number are mostly safest approach
but I guess in jest repo it doesn't matter much

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

is there a lint rule we can activate for this?

e2e/setup-files-after-env-config/setupHooksIntoRunner.js Outdated Show resolved Hide resolved
@@ -20,7 +20,7 @@ assert(process.argv[3], 'Pass the number of iterations');

const sleep = ms => new Promise(resolve => setTimeout(resolve, ms));
const method = process.argv[2];
const calls = +process.argv[3];
const calls = Number(process.argv[3]);
Copy link
Member

Choose a reason for hiding this comment

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

should use parseInt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes parseInt seems more appropraite here

@Biki-das
Copy link
Contributor Author

is there a lint rule we can activate for this?

Let me see, if there is, than we can activate a😀

@SimenB
Copy link
Member

SimenB commented Feb 23, 2022

You need to rebase btw, after #12447

@Biki-das
Copy link
Contributor Author

@SimenB as you asked if there's a lint rule and it does exist - https://eslint.org/docs/rules/no-implicit-coercion

I will add this and then we will get other occurrence , do i need to pull a seperare PR for the rule? or do it here

@Biki-das
Copy link
Contributor Author

@SimenB i feel i have messed up this one, 😟😟, is it ok i pull a fresh one and then also add the linting rule ?

@SimenB
Copy link
Member

SimenB commented Feb 23, 2022

Just reset from main and force push your changes 🙂

@Biki-das Biki-das closed this Feb 23, 2022
@Biki-das Biki-das deleted the jest-prettier branch February 23, 2022 13:35
@Biki-das Biki-das restored the jest-prettier branch February 23, 2022 13:35
@Biki-das
Copy link
Contributor Author

@SimenB idk its not working , its already a mess ☹☹, better would be to close this?

@Biki-das
Copy link
Contributor Author

i am really really sorrry , for the last time let me pull a new one and fix the occurunces

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants