First implementation of Kernel.dbg/2 #11974

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.

Sign up for GitHub

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

Jump to bottom

Merged

whatyouhide merged 25 commits into main from andrea/start-dbg-macro

Jul 12, 2022

Member

whatyouhide commented Jul 10, 2022

No description provided.

whatyouhide requested a review from josevalim

July 10, 2022 07:19

josevalim reviewed

View reviewed changes

lib/elixir/lib/kernel.ex Outdated Show resolved Hide resolved

josevalim reviewed

View reviewed changes

lib/elixir/lib/kernel.ex Outdated Show resolved Hide resolved

josevalim reviewed

View reviewed changes

lib/elixir/lib/kernel.ex Outdated Show resolved Hide resolved

josevalim reviewed

View reviewed changes

lib/elixir/lib/kernel.ex Outdated Show resolved Hide resolved

josevalim reviewed

View reviewed changes

lib/elixir/lib/macro.ex Outdated Show resolved Hide resolved

Member

josevalim commented Jul 10, 2022

It is looking awesome, I have added some comments. I am wondering if we should call it dbg or dbg! but there is likely not much reason to have a !.

lukaszsamson reviewed

View reviewed changes

lib/elixir/lib/kernel.ex Outdated Show resolved Hide resolved

Member Author

whatyouhide commented Jul 11, 2022

@josevalim this is ready for another round of review.

whatyouhide changed the title ~~Initial work on Kernel.dbg/2~~ First implementation of Kernel.dbg/2

josevalim reviewed

View reviewed changes

lib/elixir/lib/kernel.ex Outdated Show resolved Hide resolved

josevalim reviewed

View reviewed changes

lib/elixir/lib/macro.ex Outdated

+                  quote do
+                    unquote(values_ast)
+                    {:pipe, unquote(escape(asts)), Enum.reverse(unquote(values_acc_var))}

Member

josevalim Jul 11, 2022

I am almost 100% you can convert the asts to string here, instead of escaping them.

Member Author

whatyouhide Jul 11, 2022

Done ✅

josevalim reviewed

View reviewed changes

lib/elixir/lib/macro.ex Outdated Show resolved Hide resolved

josevalim reviewed

View reviewed changes

lib/elixir/lib/macro.ex Outdated

+                # Any other AST.
+                defp dbg_ast_to_debuggable(ast) do
+                  quote do: {:value, unquote(escape(ast)), unquote(ast)}

Member

josevalim Jul 11, 2022

Same here!

Member Author

whatyouhide Jul 11, 2022

Done ✅

josevalim reviewed

View reviewed changes

lib/elixir/lib/macro.ex Show resolved Hide resolved

josevalim reviewed

View reviewed changes

Member

josevalim left a comment

Just very minor comments left. :)

fertapric reviewed

View reviewed changes

lib/elixir/test/elixir/macro_test.exs Show resolved Hide resolved

whatyouhide and others added 13 commits

July 11, 2022 15:54


          Start DBG macro

6183ee9


          One test

668c339


          More

02d3ac5


          No colors

dce86a6


          More

130ae1c


          Format

f782d93


          More tests

5997f6b


          Docs

b0d1c14


          Oooops


          Remove test file

1f63efc


          Update lib/elixir/lib/kernel.ex

f6c9115

Co-authored-by: José Valim <jose.valim@dashbit.co>


          Update lib/elixir/lib/kernel.ex

e84a90b

Co-authored-by: José Valim <jose.valim@dashbit.co>


          Rename

19b1597

whatyouhide and others added 11 commits

July 11, 2022 15:54


          Rename Macro.dbg/3

9c0a256


          Less generated code


          Nicer things

09169b4


          Use Macro.pipe/3

b5f3844


          Changes

7963e4b


          Formatting


          Better docs

a9be474


          Update lib/elixir/lib/macro.ex

80f1d89

Co-authored-by: José Valim <jose.valim@dashbit.co>


          Update lib/elixir/lib/kernel.ex

9f6fd5b

Co-authored-by: José Valim <jose.valim@dashbit.co>


          Update lib/elixir/lib/macro.ex

ae517c8

Co-authored-by: José Valim <jose.valim@dashbit.co>


          Fixes

whatyouhide commented

View reviewed changes

lib/elixir/test/elixir/macro_test.exs

Comment on lines +273 to +279

+                  defmacrop dbg_format(ast, options \\ quote(do: [syntax_colors: []])) do
+                    quote do
+                      ExUnit.CaptureIO.with_io(fn ->
+                        unquote(Macro.dbg(ast, options, __CALLER__))
+                      end)
+                    end
+                  end

Member Author

whatyouhide Jul 11, 2022

Opted to avoid the "no I/O" testing strategy for now in favor of simplicity. Testing with capturing I/O lets us keep the code in Macro simpler, with less functions involved. This should be fast enough for now. If we add a lot more tests, we can always go back to a purely-functional testing approach by reworking the new Macro functions slightly.

lib/elixir/lib/macro.ex Outdated Show resolved Hide resolved

lib/elixir/lib/kernel.ex Outdated Show resolved Hide resolved

lib/elixir/lib/kernel.ex Outdated Show resolved Hide resolved

lib/elixir/test/elixir/macro_test.exs Show resolved Hide resolved

lib/elixir/lib/macro.ex Outdated

+                  quote do
+                    unquote(values_ast)
+                    {:pipe, unquote(escape(asts)), Enum.reverse(unquote(values_acc_var))}

Member Author

whatyouhide Jul 11, 2022

Done ✅

lib/elixir/lib/macro.ex Outdated

+                # Any other AST.
+                defp dbg_ast_to_debuggable(ast) do
+                  quote do: {:value, unquote(escape(ast)), unquote(ast)}

Member Author

whatyouhide Jul 11, 2022

Done ✅

whatyouhide force-pushed the andrea/start-dbg-macro branch from 64ed871 to 6330524 Compare

July 11, 2022 14:06

whatyouhide marked this pull request as ready for review

July 11, 2022 14:39

whatyouhide requested a review from josevalim

July 11, 2022 14:39

Member Author

whatyouhide commented Jul 11, 2022

@josevalim ready for final review, I think.

josevalim approved these changes

View reviewed changes

Member

josevalim left a comment

Beautiful!!!

fertapric reviewed

View reviewed changes

lib/elixir/lib/kernel.ex Show resolved Hide resolved

fertapric approved these changes

View reviewed changes

Member

fertapric left a comment

😍


          Add docs

4f478a3

whatyouhide merged commit a8bf349 into main

whatyouhide deleted the andrea/start-dbg-macro branch

July 12, 2022 07:32

Member

wojtekmach commented Jul 12, 2022

This is amazing!

Two quick questions. For this file:

# foo.exs
dbg("foo")

"foo" |> String.upcase() |> dbg

I'm getting:

[foo.exs:1: (file)]
"foo" #=> "foo"

[foo.exs:3: (file)]
|> "foo" #=> "foo"
|> String.upcase() #=> "FOO"

Notice two blank lines after a pipe. Should there be one blank line, like everywhere else?
Notice the printed pipeline starts with |> "foo". Should it be just "foo"?

whatyouhide mentioned this pull request

Fix a couple of small bugs in Macro.dbg/3 #11982

Merged

Member

jonatanklosko commented Jul 19, 2022

@whatyouhide fantastic job!! 🐱

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

Labels

None yet