Skip to content

Add :label option to IO.inspect/2#5010

Merged
whatyouhide merged 2 commits intoelixir-lang:masterfrom
michalmuskala:inspect-tag
Jul 16, 2016
Merged

Add :label option to IO.inspect/2#5010
whatyouhide merged 2 commits intoelixir-lang:masterfrom
michalmuskala:inspect-tag

Conversation

@michalmuskala
Copy link
Copy Markdown
Member

@michalmuskala michalmuskala commented Jul 15, 2016

This option will decorate the output with additional string, so
it's easy to distinguish between different IO.inspect calls.

IO.inspect("foo", tag: "bar")
#=> bar: "foo"

Closes #5009

Comment thread lib/elixir/lib/io.ex Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"of formatting options" -> "of the remaining formatting options"?

@whatyouhide whatyouhide changed the title Add :tag option to IO.inspect Add :tag option to IO.inspect/2 Jul 15, 2016
Comment thread lib/elixir/lib/io.ex Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use #=> for return values, so those should be reversed or something.

@michalmuskala
Copy link
Copy Markdown
Member Author

@whatyouhide I tried improving the docs for whole IO.inspect/2, wdyt?

Comment thread lib/elixir/lib/io.ex Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome with the before/after example, I love it. About the format though, what if we do:

@doc """
...

## Examples

    IO.inspect <<0, 1, 2>>, width: 40
    # Prints:
    # <<0, 1, 2>>

    IO.inspect 1..100, tag: "a wonderful range"
    # Prints:
    # a wonderful range: 1..100

without showing the return value (which is what we already do?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this

@michalmuskala michalmuskala force-pushed the inspect-tag branch 2 times, most recently from e07fe65 to 904194f Compare July 15, 2016 10:55
Comment thread lib/elixir/lib/io.ex Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't say this in other docs. It's structured like:

expression
#=> output

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this Prints: looks like part of actual output.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, that's confusing: #5010 (comment)

We use #=> for return values, so those should be reversed or something.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lexmag @whatyouhide maybe something like this will be clearer:

      [1, 2, 3]
      |> IO.inspect(tag: "before")
      |> Enum.map(&(&1 * 2))
      |> IO.inspect(tag: "after")
      |> Enum.sum

      Prints:
      # before: [1, 2, 3]
      # after: [2, 4, 6]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we had same discussion when warn function was introduced and decided to go with #=> until we find a better option.

@michalmuskala that Prints: now looks like a part of the code, what if we inline this into comment like:

      [1, 2, 3]
      |> IO.inspect(tag: "before") # prints "before: [1, 2, 3]"
      |> Enum.map(&(&1 * 2))
      |> IO.inspect(tag: "after") # prints "after: [2, 4, 6]"
      |> Enum.sum

?

Copy link
Copy Markdown
Member

@whatyouhide whatyouhide Jul 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am definitely 👎 on using #=> because we use if for return values (and we should fix IO.warn/2 and friends as well). I like @michalmuskala's suggestion of showing the whole snippet of code (with the return value of the whole pipe after #=> maybe) and then something like:

"""
    [1, 2, 3]
    |> ...
    #=> ...

Prints:

    before: [1, 2, 3]
    after: [2, 4, 6]
"""

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option is to not doctest this module but keep examples exactly as users would see in the terminal. For example:

iex> [1, 2, 3] |>
...> IO.inspect(tag: "before") |>
...> Enum.map(&(&1 * 2)) |>
...> IO.inspect(tag: "after") |>
...> Enum.sum
before: [1, 2, 3]
after: [2, 4, 6]
12

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@josevalim that would work but we would lose the doctests (even if that may be ok, in general it's not good I think 😕) and also still not have a solution for this. I think the two separate blocks of code may be good here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two separate blocks are good option I think 👍.

@whatyouhide
Copy link
Copy Markdown
Member

So @michalmuskala, to recap:

  • let's go with :label instead of :tag (as just decided in Support a :label option in IO.inspect/2 #5009)
  • let's use two separate code blocks to show what the code in the first and what IO.inspect/2 prints in the second, as you showed in your example above

and we should be good to go :)

@michalmuskala michalmuskala force-pushed the inspect-tag branch 2 times, most recently from 3931dcb to 226ef51 Compare July 15, 2016 14:31
@michalmuskala michalmuskala changed the title Add :tag option to IO.inspect/2 Add :label option to IO.inspect/2 Jul 15, 2016
@michalmuskala
Copy link
Copy Markdown
Member Author

@whatyouhide ready for review

@whatyouhide
Copy link
Copy Markdown
Member

@michalmuskala now that I'm seeing it in action, I think we should drop the #=> altogether 😕 It makes things a bit confusing IMO and we already said that this function returns its argument so there's not too much value in showing it everytime IMO. Wdyt?

@michalmuskala
Copy link
Copy Markdown
Member Author

@whatyouhide removed

@whatyouhide
Copy link
Copy Markdown
Member

This looks fantastic to me, and ready to merge :) @josevalim do I :shipit:?

Comment thread lib/elixir/lib/io.ex Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"This makes it possible to "spy" on values by inserting an IO.inspect/2 call almost anywhere in your code, even in the middle of a pipeline.", might be better phrasing?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the "even" like using IO.inspect/2 in a pipeline was special, but 👍 on the rest of the phrasing, may be a bit better :) But overall I think we're ok to merge, these are mostly details for now, otherwise we're gonna be torturing @michalmuskala with fixes over fixes 😄

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not bad as is, but currently this does not read as idiomatic English, imo. :) Just figured this is some documentation people new to the language might see frequently, so it should be as good as possible.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up to you @michalmuskala :)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes it possible to "spy" on values by inserting an
IO.inspect/2 call almost anywhere in your code, for example,
in the middle of a pipeline.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

@ericentin
Copy link
Copy Markdown
Contributor

ericentin commented Jul 15, 2016

I love this. Would anyone be in support of adding a short name for the :label option, perhaps :l?

@whatyouhide
Copy link
Copy Markdown
Member

@antipax strong 👎 for my side for :l. More complexity and more crypticness, no need to introduce either IMO 😄

@ericentin
Copy link
Copy Markdown
Contributor

ericentin commented Jul 15, 2016

@whatyouhide that's all quite fair. :) The idea behind the short version was being able to add inspects quickly, but label isn't particularly long either. :)

@michalmuskala
Copy link
Copy Markdown
Member Author

@antipax @whatyouhide updated

@whatyouhide
Copy link
Copy Markdown
Member

Looks great to me. Let's wait for the official seal of approval™ :)

Comment thread lib/elixir/lib/io.ex Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Else should probably use an empty list instead of an empty binary. It is a small detail but since we are working with iodata, that makes more sense as an empty value.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems we're actually working with chardata here, and it would be better to rename iodata accordingly.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So let's rename iodata below to chardata and replace "" with [] @michalmuskala?

@josevalim
Copy link
Copy Markdown
Member

Looks good to me!

This option will decorate the output with additional string, so
it's easy to distinguish between different IO.inspect calls.

IO.inspect("foo", label: "bar")
=> bar: "foo"
@michalmuskala
Copy link
Copy Markdown
Member Author

@whatyouhide updated

@whatyouhide whatyouhide merged commit d6513a7 into elixir-lang:master Jul 16, 2016
@whatyouhide
Copy link
Copy Markdown
Member

Beautiful, thanks @michalmuskala 💟 I already love this feature :D

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.

7 participants