This repository has been archived by the owner on Apr 11, 2024. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 334
Removed inlining of fn/setup/teardown inside a try/catch #104
Open
jayphelps
wants to merge
1
commit into
bestiejs:main
Choose a base branch
from
jayphelps:trycatchwut
base: main
Could not load branches
Branch not found: {{ refName }}
Could not load tags
Nothing to show
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jdalton
force-pushed
the
master
branch
6 times, most recently
from
January 4, 2016 04:43
fbdc4d3
to
ed61695
Compare
jdalton
force-pushed
the
master
branch
6 times, most recently
from
January 28, 2016 08:10
17cca63
to
0729f71
Compare
jdalton
force-pushed
the
master
branch
5 times, most recently
from
May 23, 2016 17:50
be29f7c
to
3ce818c
Compare
jdalton
force-pushed
the
master
branch
2 times, most recently
from
July 18, 2016 17:00
bb3864c
to
b5cb520
Compare
jdalton
force-pushed
the
master
branch
2 times, most recently
from
July 24, 2016 22:34
3aca059
to
189eda5
Compare
jdalton
force-pushed
the
master
branch
5 times, most recently
from
August 2, 2016 03:11
da00022
to
e156ebc
Compare
jdalton
force-pushed
the
master
branch
2 times, most recently
from
August 8, 2016 16:47
b08353e
to
f118abf
Compare
jdalton
force-pushed
the
master
branch
3 times, most recently
from
September 26, 2016 04:41
572f88b
to
10319e4
Compare
jdalton
force-pushed
the
master
branch
4 times, most recently
from
October 6, 2016 15:30
7d00d55
to
115bc9d
Compare
jdalton
force-pushed
the
master
branch
4 times, most recently
from
November 22, 2016 05:35
2f014b6
to
c732675
Compare
jdalton
force-pushed
the
master
branch
2 times, most recently
from
December 24, 2016 15:51
f263303
to
a37c36c
Compare
jdalton
force-pushed
the
master
branch
2 times, most recently
from
February 6, 2017 06:03
fa368bc
to
7c31726
Compare
jdalton
force-pushed
the
master
branch
2 times, most recently
from
March 6, 2017 02:46
e87b3c3
to
70a8b8b
Compare
I think, than the purpose was to insert a common code to the benchmark, so that you do not have to repeat it. If you want to include the code in the measurement, you will use the options |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While getting accustomed to the code I noticed an oddity.
I'm unsure why, if it's checked as a function, fn/setup/teardown is then inlined and wrapped in a try/catch, with the catch then calling it as a function. When running the tests, I found no case where it seemed to be intentional. There may be a rare case it doesn't throw and works as expected (with different scoping rules), but I can't see a benefit to it.
Removing it makes it easier for someone to make the hooks async; possibly myself..as I'm experimenting, but no promises.
I traced the code back to the first introduction of this type of logic, here bb1511c but it's still unclear to me why it's needed since if it's not a function, it would be inlined in the
else
clause. Certainly, please correct me if I'm wrong. The tests still pass, so I would argue there's some missing tests, if this behavior is needed. 馃嵒