Skip to content

Conversation

@dkuku
Copy link
Contributor

@dkuku dkuku commented Oct 7, 2024

My take on the dbg for with - previous discussion is here: #12477
I feel that without dbg for with clauses the dbg feature is incomplete:
I started off with @sabiwara code but I took a different approach.
A with clause can have 3 possible ways of finishing:

  • do block is executed
  • one of the else blocks is executed
  • no else block is specified but value is unmatched

In with expression only the lines with <- can be unmatched so I focused only on these:
Example code:

            with {:ok, width} <- Map.fetch(opts, :width),                                                                                                                     
                 {:ok, height} <- Map.fetch(opts, :height) do                                                                                                                 
              width * height                                                                                                                                                  
            else                                                                                                                                                              
              :error -> 0                                                                                                                                                     
            end 

Currently the dbg does not print the whole code, just the important parts so the result is quite short:

  • do block is executed
All with clauses matched, result:                                                                                                                                                                                                                            
{:ok, 300}
  • no else block is specified - I'm adding one in this case to catch the result and the printed result is
With clause unmatched on line 398                                                                                                                              
{:ok, height} <- Map.fetch(opts, :height) #=> :error
  • one of the else blocks is executed - we need to know where it was unmatched in the <- section and then when it matched in -> section - the printed result in this case is:
With clause unmatched on line 419                                                                                                                              
{:ok, height} <- Map.fetch(opts, :height) #=> :error                                                                                                           
Then else clause matched on line 423                                                                                                                   
->(:error, 0) #=> 0

The last one should be :error -> 0 - I need to check how to format this prorperly.

@dkuku dkuku changed the title implement_dbg_for_with dbg for with expressions Oct 7, 2024
@sabiwara
Copy link
Contributor

sabiwara commented Oct 7, 2024

Thank you @dkuku for reopening the discussion 💜

A couple of comments from me:

one of the else blocks is executed

Having several else blocks is officially an anti-pattern so I don't think dbg needs to add more complexity to support it. Knowing the value of the unmatched clause and the final result should be enough?

I haven't tried, but I think that this approach might give an incorrect value for WithClauseError when the else clause doesn't match, since you are wrapping with tuples?

Behavior-wise, showing only the non-matching clause as suggested by José here is probably giving the most important piece of information, but my feeling is still that the behavior of the original PR is more helpful and consistent since it decomposes each step like |> pipeline.
@josevalim was the concern at the time more on the targeted result, or on the use of process dict (if it's the latter I could reopen it and fix it)

quote do
[
unquote(:->)(
[{unquote({:unmatched_ast, [], Elixir}), unquote({:result, [], Elixir})}],
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI: You typically want to use unique_var for this, there are other examples in the dbg code around.

@dkuku
Copy link
Contributor Author

dkuku commented Oct 8, 2024

Thanks for the review—you’re right, handling else isn’t really needed most of the time since the match clauses are usually simple enough.

I just wanted to reopen this because with clauses can sometimes be hard to debug, and it’s the last puzzle piece missing in dbg. I’m adding a catch-all above the other clauses to log the result when there are multiple else clauses, or logs with numbers between lines to identify which one fails.

Maybe we should publicly ask how people think this should be handled? I know there are always strong opinions, but we could run a poll. What do you think, @josevalim

@sabiwara
Copy link
Contributor

sabiwara commented Oct 9, 2024

To continue the discussion, I tried to reimplement my old PR without the process dict to see what it would mean:
#13891

The support for guards and WithClauseError makes this quite complex, so I understand José's concerns.

assert formatted =~ "With clause unmatched on line"
assert formatted =~ "{:ok, height} <- Map.fetch(opts, :height) #=> :error"
assert formatted =~ "Then else clause matched on line"
assert formatted =~ "->(:error, 0) #=> 0"
Copy link
Member

Choose a reason for hiding this comment

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

This is being formatted incorrectly.

@josevalim
Copy link
Member

I would start with baby steps. Let's add the information about which clause failed (and why) and then return the overall result. We put this out there and collect feedback. If we know which clause failed, finding out the else clause should be straight-forward in almost all with use cases.

@dkuku dkuku closed this Oct 11, 2024
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.

3 participants