Skip to content
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement a new error strategy for simplifying error messages #529

Merged
merged 2 commits into from
Jul 9, 2024

Conversation

jimidle
Copy link
Contributor

@jimidle jimidle commented Jul 4, 2024

Here we implement a new error strategy for constructing error messages such that they are actually useful instead of listing out 780 possible tokens that were expected.

@jimidle jimidle added the internal technical pr's not end user facing label Jul 4, 2024
Copy link

github-actions bot commented Jul 4, 2024

Coverage tests results

394 tests  ±0   357 ✅ ±0   4s ⏱️ ±0s
  2 suites ±0     2 💤 ±0 
  2 files   ±0    35 ❌ ±0 

For more details on these failures, see this check.

Results for commit da25f59. ± Comparison against base commit 3df786c.

♻️ This comment has been updated with latest results.

@jimidle jimidle requested review from vil1 and nfx July 4, 2024 22:38
@jimidle jimidle marked this pull request as ready for review July 4, 2024 22:39
@jimidle jimidle requested a review from a team as a code owner July 4, 2024 22:39
@jimidle jimidle enabled auto-merge July 4, 2024 22:39
Copy link
Contributor

@nfx nfx left a comment

Choose a reason for hiding this comment

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

this definitely makes errors more digestible. few nits to make it a bit more idiomatic.

msg.append(" was unexpected ")
msg.append(generateMessage(recognizer, e))
msg.append("\nexpecting one of: ")
msg.append(buildExpectedMessage(recognizer, e.getExpectedTokens))
Copy link
Contributor

Choose a reason for hiding this comment

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

use interpolated stings: val msg = s"${getTokenErrorDisplay(e.getOffendingToken)} was unexpected ..." - more readable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will. do - @vil1 also suggested the same

Comment on lines +46 to +47
val stack = e.getStackTrace
stack.foreach { traceElement =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
val stack = e.getStackTrace
stack.foreach { traceElement =>
val messages = e.getStackTrace.flatMap { traceElement => ...

flatMap would be more idiomatic here.

* @return
* the expected string with tokens renamed in more human friendly form
*/
import scala.collection.mutable
Copy link
Contributor

Choose a reason for hiding this comment

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

can you move the import to the top?

@nfx nfx disabled auto-merge July 9, 2024 16:05
@nfx nfx merged commit bcfcb34 into main Jul 9, 2024
6 checks passed
@nfx nfx deleted the feature/simplifyid branch July 9, 2024 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal technical pr's not end user facing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants