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

Fix use of floating number for the timeout and forceKillAfterTimeout options #431

Merged
merged 7 commits into from Jul 6, 2020

Conversation

Kikobeats
Copy link
Contributor

When the timeout or forceKillAfterTimeout a float number is causes an error that can be avoided

@Kikobeats
Copy link
Contributor Author

Ok I can see it's an intentional behavior

   64:   test('`forceKillAfterTimeout` should not be a float', t => {    

not sure why 🤔

@sindresorhus
Copy link
Owner

I don't remember. Maybe try Git blame.

@Kikobeats
Copy link
Contributor Author

the code was refactored several times but isInteger was introduced here:
0bd5596#diff-168726dbe96b3ce427e7fedce31bb0bcR246

I couldn't determinate if number over float is necessary for a thing, maybe @ehmicky can know 🙂

@ehmicky
Copy link
Collaborator

ehmicky commented Jun 10, 2020

Yes, I think we should allow floats.
However we should still disalllow NaN and Infinity, so Number.isFinite() might be better than typeof value === 'number' I believe?

@sindresorhus
Copy link
Owner

Yes, let’s do that.

lib/kill.js Outdated Show resolved Hide resolved
Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
lib/kill.js Outdated Show resolved Hide resolved
Co-authored-by: ehmicky <ehmicky@users.noreply.github.com>
lib/kill.js Outdated Show resolved Hide resolved
Co-authored-by: ehmicky <ehmicky@users.noreply.github.com>
@ehmicky
Copy link
Collaborator

ehmicky commented Jun 11, 2020

The CI test are failing due to the following test:

test('`forceKillAfterTimeout` should not be a float', t => {

This probably should be changed to check for NaN instead?

@Kikobeats
Copy link
Contributor Author

@ehmicky test updated; still, some tests are failing but for a different reason no related with this PR

@ehmicky
Copy link
Collaborator

ehmicky commented Jun 15, 2020

Thanks @Kikobeats,

It looks like you accidentally also removed the test that checks that the argument is not negative. Could you please add it back? Thanks :)

@Kikobeats
Copy link
Contributor Author

@ehmicky ops, done!

Copy link
Collaborator

@ehmicky ehmicky left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@sindresorhus sindresorhus changed the title use typeof validation for numbers Fix use of floating number for the timeout and forceKillAfterTimeout options Jul 6, 2020
@sindresorhus sindresorhus merged commit 9a157b3 into sindresorhus:master Jul 6, 2020
@Kikobeats Kikobeats deleted the patch-1 branch July 6, 2020 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants