Skip to content

Conversation

@mauro-oto
Copy link
Contributor

Added support for some cases where the label is not shown, specifically when
using "naked" braces/brackets, see #84

Extracted a Banner class, based on
#85, and corrected the labels to
show the right matching/non-matching character.

Added support for some cases where the label is not shown, specifically when
using "naked" braces/brackets, see #84

Extracted a `Banner` class, based on
#85, and corrected the labels to
show the right matching/non-matching character.
@mauro-oto mauro-oto requested a review from schneems October 16, 2021 19:38
@mauro-oto mauro-oto marked this pull request as ready for review October 18, 2021 18:51
Copy link
Collaborator

@schneems schneems left a comment

Choose a reason for hiding this comment

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

High level, it looks good. I'm asking that you the character reversing logic be moved into the "who dis" class and eliminate the extra state that the Banner class.

Short term I want to merge this in. I love the banner class extraction and I think it's a step in the right direction. Long term I've want to share some context and some extra thoughts. I'll drop those thoughts in the comment section.

I'm curious on your thoughts there.

else
"DeadEnd: Unmatched `#{missing_character}` character detected"
end
when :unmatched_syntax
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm realizing that unmatched_syntax and unexpected_syntax are very similar. I'm thinking that we should make them more unique at a glance maybe missing_symbol and unexpected_syntax ? What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually scratch that, If we normalize and reverse characters in "who dis" instead then we won't need to add this extra state. I would prefer it that way instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! There's still one reversal I do here, in missing_character to show which character might be missing, see the following example:

On 2.0 release:

➜  dead_end git:(mauro-oto/support-naked-braces-brackets-and-invert-labels) ✗ dead_end test1.rb

DeadEnd: Unmatched `]` detected

file: /Users/motonelli/Projects/dead_end/test1.rb
simplified:

       1  module Hey
       2    class Foo
       3      def initialize
    ❯  4        [1,2,3
       5      end
       9    end
      10  end

On this branch:

➜  dead_end git:(mauro-oto/support-naked-braces-brackets-and-invert-labels) ✗ exe/dead_end test1.rb

DeadEnd: Unmatched `[` character detected

It appears a `]` is missing.

file: /Users/motonelli/Projects/dead_end/test1.rb
simplified:

       1  module Hey
       2    class Foo
       3      def initialize
    ❯  4        [1,2,3
       5      end
       9    end
      10  end

(For the It appears a ']' is missing. message, which is part of the banner)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool, works for me

"DeadEnd: Unmatched `#{@invalid_obj.unmatched_symbol}` detected"
end
end
Banner.new(invalid_obj: @invalid_obj).call
Copy link
Collaborator

Choose a reason for hiding this comment

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

Love this. Extracting this logic to it's own class makes so much more sense.

@schneems
Copy link
Collaborator

schneems commented Oct 18, 2021

Long-term context and thoughts.

Context

  • There can be multiple syntax errors in one document. I wrote the "who dis" logic mostly only assuming one.
  • The detection that "who dis" provides us is at the document level, instead of the individual "block" level. I.e. if there are multiple syntax errors across multiple blocks, there will only be the same suggestion for all of them
  • The error messages seem very inconsistent, for instance, it's not guaranteed that ] will even show in the output:
$ cat << EOM | ruby -wc
1,2,3]
EOM
-:1: warning: possibly useless use of a literal in void context
-:1: syntax error, unexpected ',', expecting end-of-input
1,2,3]

(See how it complains about unexpected , instead of the bracket)

Thoughts moving forward

Long term I think we need to:

  • Remove the logic that checks for end-of-output before firing dead_end
  • Specialize each block's output to only show errors that occur in that block
  • Allow each block to list multiple error causes (requires capturing multiple error causes).
  • I am betting that if we move away from grepping the error message to detecting problems ourselves, we'll get better and more consistent results. For example: using lex to scan for mis-matched brackets, parens, curly brackets. We can also use lex to identify too many or too few end statements. The reason I think this will work is that we know that there's invalid code and a syntax error in the block that was captured so we've already narrowed the search.

I'm sure when we start doing this there will be new syntax errors we weren't expecting like:

$ cat << EOM | ruby -wc
"hello" , "world"
EOM
-:1: warning: possibly useless use of a literal in void context
-:1: syntax error, unexpected ',', expecting end-of-input
"hello" , "world"

We can handle the ones that make sense, and fall back on the parser's error message for ones we don't explicitly annotate.

Far far future

Here's an example of rust output showing multiple problems (that are all related), rust-lang/rust@dc003dd#diff-48cbc5a282328584a4a369f01e7c3fd128b2aa9f47869f0d04048795963530c9. I think long-long term it would be really cool to start being able to not just suggest "you need a bracket to make this code happy" but to inspect the code, find the commas, and suggest the bracket go there. Before we can get there we need to get the "here's why your code is failing" suggestion logic to be rock-solid.


expect(
DeadEnd.invalid_type(source).unmatched_symbol
).to eq(:"[")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@schneems I realized this was wrong even in the previous commit, now it shows up correctly "reversed", as shown in https://github.com/zombocom/dead_end/pull/89/files#r731862726, shows the symbol that failed to match, and what the missing character could be.

mauro-oto and others added 2 commits October 19, 2021 10:30
Copy link
Collaborator

@schneems schneems left a comment

Choose a reason for hiding this comment

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

Awesome sauce, thanks!

@schneems schneems merged commit 036df50 into main Oct 19, 2021
@mauro-oto mauro-oto deleted the mauro-oto/support-naked-braces-brackets-and-invert-labels branch October 19, 2021 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants