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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Formatter option tuples introduction #242

Merged
merged 9 commits into from Aug 10, 2018

Conversation

Projects
None yet
3 participants
@PragTob
Copy link
Owner

PragTob commented Aug 2, 2018

no really ready for review yet just wanted to bring it out here

  • high level integration tests (benchee_test) pass

  • still needs to fix existing unit tests and add new ones for what
    I added

  • also probably move some code around

  • fix unit tests

  • write new tests (this is just a sketch to see if the concept as a whole holds up)

  • remove the Formatter use just stick with it as a behaviour and use output from somewhere

  • fix documentation 馃挭

@PragTob PragTob force-pushed the formatter-options-closer branch from 1210763 to 2b4dd9b Aug 2, 2018

@PragTob

This comment has been minimized.

Copy link
Owner Author

PragTob commented Aug 2, 2018

lil flakey

  1) test measure/1 will not leak processes if the applied function raises an exception (Benchee.MemoryMeasureTest)
     test/benchee/benchmark/measure/memory_test.exs:30
     Assertion with == failed
     code:  assert Enum.count(Process.list()) == starting_processes
     left:  93
     right: 94
     stacktrace:
       test/benchee/benchmark/measure/memory_test.exs:43: (test)
..
Finished in 4.5 seconds
111 doctests, 235 tests, 1 failure, 4 skipped
# to do something akin to: `fn suite -> Formatter.output(suite, my_opts_here) end`
# so that it ends up in the upper part of this if
function.(suite, @default_opts)
end

This comment has been minimized.

@michalmuskala

michalmuskala Aug 2, 2018

Contributor

This will be better with:

defp call_formatter_function(function, suite) when is_function(function, 1) do
  function.(suite)
end

defp call_formatter_function(function, suite) when is_function(function, 2) do
  function.(suite, @default_opts)
end

This comment has been minimized.

@PragTob

PragTob Aug 2, 2018

Author Owner

Yay for is_function - you should review my PRs more often there seems to be a lot of room for me to learn 馃榿

{formatter, config.formatter_options[compatible_formatter_options_key]}
else
formatter
end

This comment has been minimized.

@michalmuskala

michalmuskala Aug 2, 2018

Contributor

I would write this as:

Enum.map(config.formatters, fn
  Benchee.Formatters.Console -> {formatter, config.formatter_options[:console]}
  Benchee.Formatters.CSV -> {formatter, config.formatter_options[:csv]}
  Benchee.Formatters.JSON -> {formatter, config.formatter_options[:json]}
  Benchee.Formatters.HTML -> {formatter, config.formatter_options[:html]}
  formatter -> formatter
end)

You could also get a bit fancy with elixir and do something like:

update_in(config, [Access.key(:formatters), Access.all()], fn 
  # function from above
end)

And this would unpack and pack the config struct for you.

This comment has been minimized.

@PragTob

PragTob Aug 2, 2018

Author Owner

Hm I sort of liking to keep the relationship as data and have the code be more general. However, this is highly temporary code where I hope I can delete it soooooonnnn. Then that version makes a lot more sense as it's way more condensed.

The update_in seems like... woooahhhh. I need to read up on Access module it seems.

Thanks a ton Micha艂! 馃檹


@doc """
Combines `format/1` and `write/1` into a single convenience function that is
also chainable (as it takes a suite and returns a suite).
"""
@callback output(Suite.t()) :: Suite.t()
@callback output(Suite.t(), options) :: Suite.t()

This comment has been minimized.

@michalmuskala

michalmuskala Aug 2, 2018

Contributor

What do you think about getting rid of the output/2 as a callback - it seems very weird - as far as I understand you never want to override it.

How about defining a regular Benchee.Formatter.output(formatter, suite, options) function that would do this? This could allow removing the use from Benchee.Formatter entirely and keeping it a plain & simple behaviour with no macro magic.

This comment has been minimized.

@PragTob

PragTob Aug 2, 2018

Author Owner

Thanks for your feedback @michalmuskala !

馃 Basically the output/2 callback still exists for 2 reasons:

  1. Backwards compatibility-ish (as it's now /2 but I also just had the idea to let the second arg default thereby keeping output/1)
  2. Convenience access to format + write especially for people who use the more verbose piped interface. Of course they could also just use Benchee.Formatter.output then - seems a bit weird.

Not sure, generally I like getting rid off use so that's a plus. Maybe it's just hard for me to let go 馃榿 One thing for me would be that you could no longer say &Formatter.output/arity as the formatter would always have to be a parameter

This comment has been minimized.

@PragTob

PragTob Aug 2, 2018

Author Owner

@devonestes input on our good ol friend mister output? :)

This comment has been minimized.

@devonestes

devonestes Aug 3, 2018

Collaborator

It's fine by me to get rid of it. I guess if folks wanted to change how stuff is formatted, they'd just override format/2, and if they wanted to change how or where stuff is written they'd overwrite write/2.

Let's cut it!

This comment has been minimized.

@PragTob

PragTob Aug 3, 2018

Author Owner

'Tis decided then. That requires even more updates then. I'll see how the rest shapes up and include it later in this PR or remove it as part of a separate PR (maybe more pragmatic option). Maybe I'm also overestimating it.

Ideas for names? Right now Formatter.output outputs the whole suite with all configured formatters. What should the output method for just one for formatter be called? output_one ? :D

{parallelizable, serial} =
formatters
|> Enum.map(&normalize_module_configuration/1)
|> Enum.split_with(&is_formatter_module?/1)

# why do we ignore this suite? It shouldn't be changed anyway.
_suite = Formatter.parallel_output(suite, parallelizable)

This comment has been minimized.

@devonestes

devonestes Aug 3, 2018

Collaborator

Unrelated to this change, but I bet we have this so Dialyzer doesn't complain. We can probably remove that comment asking why this is so, or just rebind suite since it should be unchanged.


@doc """
Combines `format/1` and `write/1` into a single convenience function that is
also chainable (as it takes a suite and returns a suite).
"""
@callback output(Suite.t()) :: Suite.t()
@callback output(Suite.t(), options) :: Suite.t()

This comment has been minimized.

@devonestes

devonestes Aug 3, 2018

Collaborator

It's fine by me to get rid of it. I guess if folks wanted to change how stuff is formatted, they'd just override format/2, and if they wanted to change how or where stuff is written they'd overwrite write/2.

Let's cut it!

|> Enum.map(&normalize_module_configuration/1)
|> Enum.split_with(&is_formatter_module?/1)

# why do we ignore this suite? It shouldn't be changed anyway.

This comment has been minimized.

@devonestes

devonestes Aug 3, 2018

Collaborator

This is probably because of Dialyzer complaining about an unused return value.

This comment has been minimized.

@PragTob

PragTob Aug 3, 2018

Author Owner

Oh yes that's why I added it in there, but I still thought not using requires more of an explanation 馃

This comment has been minimized.

@PragTob

PragTob Aug 3, 2018

Author Owner

Added a line to mention that though

configuration: %{
formatters: [
{FakeFormatter, %{}},
fn _ -> send(self(), {:fun, "me"}) end

This comment has been minimized.

@devonestes

devonestes Aug 3, 2018

Collaborator

Should we also test that the suite is being passed as the argument here? So, we could do fn suite -> send(self(), {:fun, suite}) end and then assert_receive {:fun, ^suite}.

This comment has been minimized.

@PragTob

PragTob Aug 3, 2018

Author Owner

good idea will add that

@PragTob

This comment has been minimized.

Copy link
Owner Author

PragTob commented Aug 3, 2018

Blah gawd with my optional second argument dialyzer isn't happy with the output method... guess gotta remove it in this PR.

incidentally this is me right now so maybe another time:

img_20180730_120143

@PragTob PragTob changed the title First sketch of formatter - option tuples Formatter option tuples introduction Aug 6, 2018

@PragTob PragTob force-pushed the formatter-options-closer branch from 89c9d40 to 50ea4f1 Aug 6, 2018

PragTob added some commits Aug 2, 2018

First sketch of formatter - option tuples
* high level integration tests (benchee_test) pass
* still needs to fix existing unit tests and add new ones for what
I added
* also probably move some code around
Move formatting logic/handling of formatters into formatter module
The Benchee module should be a rather thin module so this logic is
better contained here. As we want the behaviour to be documented
it can't be "library private" so we might have to move it again.

@PragTob PragTob force-pushed the formatter-options-closer branch from 50ea4f1 to 33ed4fb Aug 6, 2018

@PragTob

This comment has been minimized.

Copy link
Owner Author

PragTob commented Aug 6, 2018

Rebased to get rid off some bad commits and to be able to touch the changelog more easily

@PragTob

This comment has been minimized.

Copy link
Owner Author

PragTob commented Aug 6, 2018

This might be done pending your reviews 馃懠

Dreamy dog for a change of pace!

whatsapp image 2018-08-05 at 19 40 55

@devonestes
Copy link
Collaborator

devonestes left a comment

I think this is ready! Just to small additions that are really just nice-to-haves. 馃憤

@spec format(Suite.t()) :: [any]
def format(%Suite{scenarios: scenarios, configuration: config}) do
config = console_configuration(config)
@spec format(Suite.t(), map) :: [any]

This comment has been minimized.

@devonestes

devonestes Aug 7, 2018

Collaborator

We might want to consider using @impl Benchee.Formatter here (and for write/2) so we get the compiler check that we're implementing the behaviour correctly.

@spec format(Suite.t()) :: {binary, String.t()}
def format(suite = %Suite{configuration: config, scenarios: scenarios}) do
formatter_config = config.formatter_options.tagged_save
@spec format(Suite.t(), map) :: {binary, String.t()}

This comment has been minimized.

@devonestes

devonestes Aug 7, 2018

Collaborator

Same thing here about @impl Benchee.Formatter

@PragTob

This comment has been minimized.

Copy link
Owner Author

PragTob commented Aug 8, 2018

As only one behaviour is specified in those modules I decided to go with just an @impl true - it's the first time I'm using this so I might be missing something :)

@devonestes

This comment has been minimized.

Copy link
Collaborator

devonestes commented Aug 9, 2018

I think @impl Benchee.Formatter just gives slightly nicer error messages since it knows to check that behaviour. @impl true will work just fine here for sure.

I think if we want to keep Elixir 1.5 compatability, we'll need to rewrite the functions in the console formatter, though, to remove the default argument. To the compiler in 1.5, those functions with default arguments have an arity of 1, which then means it's not implementing the behaviour properly.

We can do the following to get around it:

def write(suite), do: write(suite, %{})

@impl true
def write(suite, options) do
  #...
end

I have a feeling that this is essentially what the macro expanded to when there was default arguments in 1.5. We just need to be specific here to make it happy 馃槈

@PragTob

This comment has been minimized.

Copy link
Owner Author

PragTob commented Aug 9, 2018

Grml.

I'll take a look when I get time, might want to get rid off the default argument again although it's quite nice if you wanna specify it as a function but then you shouldn't so we can just get rid off it

@PragTob

This comment has been minimized.

Copy link
Owner Author

PragTob commented Aug 10, 2018

馃挜

@PragTob PragTob merged commit 33c7ea8 into master Aug 10, 2018

5 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-1.2%) to 93.939%
Details

@PragTob PragTob deleted the formatter-options-closer branch Aug 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment