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.

Sign up for GitHub

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

Jump to bottom

removed duplicate code, requiring file instead #393

Closed
wants to merge 1 commit into from
Closed

removed duplicate code, requiring file instead #393

wants to merge 1 commit into from

Conversation

gouravtiwari
Copy link

It seems the code is duplicated, so required a file to reuse ack method here.

@zzak zzak closed this in 000f39b Sep 20, 2013
@gouravtiwari
Copy link
Author

Hey @zzak, is there anything wrong with the commit? I saw it closed instead of merged.

@zzak
Copy link
Member

zzak commented Sep 20, 2013

@gouravtiwari i committed it in 000f39b

@eregon
Copy link
Member

eregon commented Sep 21, 2013

Benchmark code is very sensitive to execution conditions, including calling require or not.
(For instance, actually loading RubyGems has a significant impact.)
Therefore I think this should not be merged.
It also introduces a dependency to files in other-lang, which might be undesirable.

@eregon eregon mentioned this pull request Sep 21, 2013
Closed
@zzak
Copy link
Member

zzak commented Sep 21, 2013

That is a very good point! Sorry! Lets revert it.

@eregon thanks for the review! <3

On Sep 21, 2013, at 8:06 AM, Benoit Daloze notifications@github.com wrote:

Benchmark code is very sensitive to execution conditions, including calling require or not.
(For instance, actually loading RubyGems has a significant impact.)
Therefore I think this should not be merged.
It also introduces a dependency to files in other-lang, which might be undesirable.

�
Reply to this email directly or view it on GitHub.

@gouravtiwari
Copy link
Author

Great point, I didn't think about it. Benchmark tests should never have dependency on any other features, which might slow the tests. THanks @zzak and @eregon :)

mmasaki pushed a commit to mmasaki/ruby that referenced this pull request Oct 6, 2013
* benchmark/bm_app_answer.rb: removed duplicate code [Fixes rubyGH-393]
4585cb4 
  ruby#393


git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@42990 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
headius pushed a commit to headius/ruby that referenced this pull request Oct 31, 2013
@eregon
* benchmark/bm_app_answer.rb: revert r42990, benchmark scripts should
3557a5f 
  be self-contained and avoid dependencies, especially such small one.
  See ruby#393 (comment).

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@43012 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
tenderlove pushed a commit to tenderlove/ruby that referenced this pull request Jan 24, 2014
* benchmark/bm_app_answer.rb: removed duplicate code [Fixes rubyGH-393]
8d3c4fc 
  ruby#393


git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@42990 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
tenderlove pushed a commit to tenderlove/ruby that referenced this pull request Jan 24, 2014
@eregon
* benchmark/bm_app_answer.rb: revert r42990, benchmark scripts should
f00afff 
  be self-contained and avoid dependencies, especially such small one.
  See ruby#393 (comment).

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@43012 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
3 participants
@gouravtiwari @zzak @eregon