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

refactor(throwError): remove utility throwError() function #1819

Merged
merged 1 commit into from
Jul 11, 2016

Conversation

kwonoj
Copy link
Member

@kwonoj kwonoj commented Jul 10, 2016

Description:

This PR removes utility function throwError, which simply wraps throw and only being used in single place. Error can be thrown directly.

Related issue (if exists):

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.001%) to 96.471% when pulling 20a0de2 on kwonoj:refactor-throwError into 3632489 on ReactiveX:master.

@kwonoj
Copy link
Member Author

kwonoj commented Jul 10, 2016

Not sure why coverage's decreased :/

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 96.501% when pulling 97819eb on kwonoj:refactor-throwError into 4971993 on ReactiveX:master.

@jayphelps
Copy link
Member

Was this being done for performance reasons? I'm not aware of any, but wouldn't be surprised if wrapping the throw inside a function some how prevented a deopt or something. @Blesh ?

@kwonoj
Copy link
Member Author

kwonoj commented Jul 10, 2016

Was this being done for performance reasons? I'm not aware of any

I was also bit curious, sending out PR as discussion point, especially this has been used in only single place of codebase.

@coveralls
Copy link

coveralls commented Jul 10, 2016

Coverage Status

Coverage decreased (-0.001%) to 96.471% when pulling 26685b1 on kwonoj:refactor-throwError into 19f4362 on ReactiveX:master.

@benlesh
Copy link
Member

benlesh commented Jul 11, 2016

TMK, throw doesn't cause a deopt. I'm not sure why this function existed.

Need to make sure the code coverage stays up, other than that, it LGTM

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.001%) to 96.471% when pulling bdb9dbd on kwonoj:refactor-throwError into 19f4362 on ReactiveX:master.

@jayphelps
Copy link
Member

coveralls doesn't make sense to me. It says in the list of changes that there was no change in coverage, but then claims -0.001% overall change. Seems like a rounding error or something...

@jayphelps
Copy link
Member

jayphelps commented Jul 11, 2016

LGTM

@kwonoj
Copy link
Member Author

kwonoj commented Jul 11, 2016

coveralls doesn't make sense to me. It says in the list of changes that there was no change in coverage, but then claims -0.001% overall change. Seems like a rounding error or something

I assume similar, some kind of threshold between line changes. Tried dummy changes but no luck. :/
Let me check this in once current travis result comes back. Thanks for review @Blesh , @jayphelps .

@coveralls
Copy link

coveralls commented Jul 11, 2016

Coverage Status

Coverage decreased (-0.001%) to 96.471% when pulling bdb9dbd on kwonoj:refactor-throwError into 19f4362 on ReactiveX:master.

@jayphelps jayphelps merged commit ffcb9fc into ReactiveX:master Jul 11, 2016
@kwonoj kwonoj deleted the refactor-throwError branch July 11, 2016 17:57
@lock
Copy link

lock bot commented Jun 6, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants