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

adds Tms.to_h #4

Merged
merged 3 commits into from
Feb 8, 2021
Merged

adds Tms.to_h #4

merged 3 commits into from
Feb 8, 2021

Conversation

keithrbennett
Copy link
Contributor

Adds a to_h method to Benchmark::Tms.

Fixes the test file's require to load the code in the modified benchmark.rb instead of the Ruby distribution.

@hsbt hsbt changed the title #3 adds Tms.to_h, fixes test require. adds Tms.to_h, fixes test require. Jan 30, 2021
@marcandre
Copy link
Member

This seems like a worthy addition. Could you please remove the require change? I am not sure how you are running the tests but it shouldn't be necessary (and is not related)

@marcandre
Copy link
Member

@keithrbennett
Copy link
Contributor Author

This seems like a worthy addition. Could you please remove the require change? I am not sure how you are running the tests but it shouldn't be necessary (and is not related)

I was running the tests on the command line using ruby test/benchmark/test_benchmark.rb. For this, the current require 'benchmark' ignores the file and uses the benchmark code included in the Ruby distribution. Maybe no one else ever does this though?

I realize that rake test works with require 'benchmark', but it would be nice to also support the ruby test/... approach too. I can change it back, but given that it wouldn't work with ruby test/..., do you still want me to? Is there an advantage to using require rather than require_relative, or is it just to remain consistent with other code (which I agree could be a legitimate reason)?

@marcandre
Copy link
Member

ruby test/..., do you still want me to?

You can use ruby -I lib test/....

Is there an advantage to using require rather than require_relative

While require_relative is usually best, a require here allows to run a test on an alternative implementation ruby -I other/path/lib test/.... In short, some people believe you should not require_relative into lib, just within it (or within test/). E.g.: https://docs.rubocop.org/rubocop-packaging/cops_packaging.html#packagingrequirehardcodinglib

@keithrbennett
Copy link
Contributor Author

@marcandre Thanks very much for the informative explanation! I've reverted that change.

@marcandre marcandre merged commit 6b94aaf into ruby:master Feb 8, 2021
@keithrbennett keithrbennett changed the title adds Tms.to_h, fixes test require. adds Tms.to_h. Feb 8, 2021
@keithrbennett keithrbennett changed the title adds Tms.to_h. adds Tms.to_h Feb 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants