Allow Suite To be Set via Accessor #113

allcentury · 2021-03-17T22:04:18Z

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

this looks nice.

not a fan of the set_reporter method but the removal of the if statements sure makes this nicer.

lib/benchmark/ips/job.rb

kbrock · 2021-03-19T23:15:01Z

lib/benchmark/ips/job.rb

+        end
+      end
+
+      def set_reporter(quiet:)


since this only accepts quiet, does it make sense to just move the contents of this over to def quiet=

or do you have plans on allowing a report to be passed into here in the future?
If you wanted that, you could do quiet=true or reporter=NoopReport.new

Great points. I was thinking reporter should eventually use dependency injection and if no report is passed in then we'll default to Noop. Then, we can actually decommission quiet from the public API and instead, if you don't want output you pass in the NoopReport. I think the idea of a reporter and quiet are conflating the same thing but getting rid of quiet seemed like a breaking change at this time.

Maybe the answer is to have a public reporter= method that self.quiet calls and eventually we can deprecate it? thoughts on that?

I like that idea a lot. it is consistent with your current approach and the approach this project seems to be headed.
At one time, quiet made sense. But now that people want more control over capturing the output, introducing a reporter accessor sounds good.

lib/benchmark/ips/job.rb

allcentury

Thanks for the review @kbrock !

lib/benchmark/ips/job.rb

allcentury · 2021-03-22T17:58:42Z

lib/benchmark/ips/job.rb

+        end
+      end
+
+      def set_reporter(quiet:)


Great points. I was thinking reporter should eventually use dependency injection and if no report is passed in then we'll default to Noop. Then, we can actually decommission quiet from the public API and instead, if you don't want output you pass in the NoopReport. I think the idea of a reporter and quiet are conflating the same thing but getting rid of quiet seemed like a breaking change at this time.

kbrock

This is looking nice.
I like where you are going with reporter=

But please note, that while I have contributed a few PRs here, I am not the maintainer so take my words with a grain of salt.

I have put in a few PRs around better capturing the output, that is why I'm vocal here. A number of people have expressed the desire to have better output / hence another pr that I have open. this seems to overlap that one a bit - so keep it coming and I'll refactor that one.

kbrock · 2021-03-22T23:02:04Z

lib/benchmark/ips/job.rb

+        end
+      end
+
+      def set_reporter(quiet:)


I like that idea a lot. it is consistent with your current approach and the approach this project seems to be headed.
At one time, quiet made sense. But now that people want more control over capturing the output, introducing a reporter accessor sounds good.

lib/benchmark/ips/job.rb

kbrock · 2021-03-30T22:46:52Z

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 · 2021-03-31T14:05:53Z

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

allcentury · 2021-03-31T20:54:45Z

@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:

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

option B:

Keep quiet and don't surface the reporter idea
Nothing to deprecate/change
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:

Sends everything to stdout

NoopReport:

No output

:format

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 · 2021-04-01T00:09:42Z

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 #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 · 2021-04-02T00:25:08Z

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 · 2021-04-02T15:42:20Z

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

kbrock

I like all but that one method.
since it is only used in the one spot, this seems a simple change

but obviously, I defer to nick on this all (since he is supporting the gem after all)

kbrock · 2021-04-02T22:29:08Z

lib/benchmark/ips/job.rb

+        @suite = suite || Benchmark::IPS::NoopSuite.new
+      end
+
+      def set_reporter(quiet:)


I think this is the only part that is suspect.

introducing report= is ok - or not. Just use that and set the value based upon quiet.
but the set_reporter doesn't seem right to me

@allcentury Purely from a style perspective, get_ and set_ methods don't belong in Ruby for me. It looks like PHP or JS. I think this should just be reporter=. I have no problem with what the method does.

nateberkopec · 2021-04-05T14:54:21Z

lib/benchmark/ips/job.rb

+        @suite = suite || Benchmark::IPS::NoopSuite.new
+      end
+
+      def set_reporter(quiet:)


@allcentury Purely from a style perspective, get_ and set_ methods don't belong in Ruby for me. It looks like PHP or JS. I think this should just be reporter=. I have no problem with what the method does.

nateberkopec · 2021-04-05T14:54:35Z

1 rename and it's GTG

This aims to close evanphx#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.

allcentury · 2021-04-05T15:19:37Z

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

nateberkopec · 2021-04-05T15:39:38Z

Thank you @allcentury

kbrock · 2021-04-06T23:10:11Z

thank you @allcentury

allcentury mentioned this pull request Mar 17, 2021

Slow Tests #114

Closed

nateberkopec added the enhancement label Mar 17, 2021

kbrock approved these changes Mar 19, 2021

View reviewed changes

kbrock mentioned this pull request Mar 19, 2021

Report to stream #111

Merged

allcentury commented Mar 22, 2021

View reviewed changes

kbrock reviewed Mar 22, 2021

View reviewed changes

allcentury force-pushed the suite-config branch from 46607e5 to 7658ae1 Compare April 2, 2021 15:35

kbrock reviewed Apr 2, 2021

View reviewed changes

nateberkopec requested changes Apr 5, 2021

View reviewed changes

allcentury force-pushed the suite-config branch from 7658ae1 to 416b0af Compare April 5, 2021 15:17

nateberkopec merged commit bc84a73 into evanphx:master Apr 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Suite To be Set via Accessor #113

Allow Suite To be Set via Accessor #113

allcentury commented Mar 17, 2021

kbrock left a comment

kbrock Mar 19, 2021

allcentury Mar 22, 2021

allcentury Mar 22, 2021

kbrock Mar 22, 2021

allcentury left a comment

allcentury Mar 22, 2021

kbrock left a comment

kbrock Mar 22, 2021

kbrock commented Mar 30, 2021

nateberkopec commented Mar 31, 2021

allcentury commented Mar 31, 2021 •

edited

kbrock commented Apr 1, 2021

nateberkopec commented Apr 2, 2021

allcentury commented Apr 2, 2021

kbrock left a comment

kbrock Apr 2, 2021

nateberkopec Apr 5, 2021

nateberkopec Apr 5, 2021

nateberkopec commented Apr 5, 2021

allcentury commented Apr 5, 2021

nateberkopec commented Apr 5, 2021

kbrock commented Apr 6, 2021

Allow Suite To be Set via Accessor #113

Allow Suite To be Set via Accessor #113

Conversation

allcentury commented Mar 17, 2021

kbrock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allcentury left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbrock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbrock commented Mar 30, 2021

nateberkopec commented Mar 31, 2021

allcentury commented Mar 31, 2021 • edited

kbrock commented Apr 1, 2021

nateberkopec commented Apr 2, 2021

allcentury commented Apr 2, 2021

kbrock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nateberkopec commented Apr 5, 2021

allcentury commented Apr 5, 2021

nateberkopec commented Apr 5, 2021

kbrock commented Apr 6, 2021

allcentury commented Mar 31, 2021 •

edited