search

evanphx / benchmark-ips

Provides iteration per second benchmarking for Ruby
MIT License
1.72k stars 97 forks source link

Allow Suite To be Set via Accessor #113

Closed allcentury closed 3 years ago

allcentury commented 3 years ago

This aims to close #112.

I removed references to Benchmark::Suite - I couldn't find anywhere this was used and the original commit goes back a long ways. This looks like a relic to a class or module that I'm not able to find in git history via

git grep 'Suite' $(git rev-list --all)

All config gets passed into job.config job_opts and now the block is evaluated last allowing overrides.

kbrock commented 3 years ago

hey @allcentury do you know how to rebase and squash some of your commits together? It makes for a cleaner git history in the future. (the changes now are the history in the future... Marty McFly would be proud)

nateberkopec commented 3 years ago

Hey @allcentury - do you have more to add here or are you waiting on my input?

allcentury commented 3 years ago

@kbrock - yes, I follow the fixup flow until it's ready to merge, then I squash.

@nateberkopec - thanks! Ya, could use your input:

option A:

  1. deprecate quiet in favor of setting a reporter.
  2. If you want no output, use the NoopReporter.
  3. Replace docs on quiet with NoopReporter
  4. Eventually let anyone inject their own Reporter as long as it conforms to the interface
  5. Do we merge format + reporter? CsvReporter, JsonReporter, etc.

option B:

  1. Keep quiet and don't surface the reporter idea
  2. Nothing to deprecate/change
  3. Don't merge format config with the output config

Initially I thought reporter could allow users to write their own formatters like csv, json, etc. However I see there is a format option. As it stands w/ this PR:

StdoutReport:

  1. Sends everything to stdout

NoopReport:

  1. No output

:format

  1. Raw or Human, used by the Report classes above.

Can you think of a good reason to have users inject their own Report class? I can't without conflating format. So I'm leaning towards Option B now. Thoughts?

kbrock commented 3 years ago

Can you think of a good reason to have users inject their own Report class? I can't without conflating format. So I'm leaning towards Option B now. Thoughts?

There was a request https://github.com/evanphx/benchmark-ips/issues/81 to be able to capture the output. So as long as you could set a string output buffer or something, then that would move this gem towards solving that issue. I had started on a PR to solve that issue but I like where this PR is going and can fix the other PR after you are done here.

nateberkopec commented 3 years ago

Well, I don't have any desire to get rid of quiet. I think setting quiet should just silently swap out the reporter in the background. So I guess that makes me option B? I don't think the other decisions/points in the PR need to be solved today, maybe we merge reporters/formatters in the future but I don't think that has to be solved now.

allcentury commented 3 years ago

@nateberkopec - thanks for that. I think this is ready for your review then.

nateberkopec commented 3 years ago

1 rename and it's GTG

allcentury commented 3 years ago

@nateberkopec perfect, thank you! I removed the set and now reporter is a pure function which quiet= relies on.

nateberkopec commented 3 years ago

Thank you @allcentury

kbrock commented 3 years ago

thank you @allcentury