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.

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

chore(developer): resolve excessive-fatal-exception issue #10949

Merged
merged 3 commits into from
Mar 12, 2024
Merged

chore(developer): resolve excessive-fatal-exception issue #10949

merged 3 commits into from
Mar 12, 2024

Conversation

srl295
Copy link
Member

@srl295 srl295 commented Mar 5, 2024 •
edited
Loading

  • add an e2e test to expose this issue
  • remove the fatal exception - it doesn't add anything of value. Actual error is already propagated to the user.

#10894

@keymanapp-test-bot skip

@srl295 srl295 self-assigned this Mar 5, 2024
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Mar 5, 2024 •
edited
Loading

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

@keymanapp-test-bot keymanapp-test-bot bot added this to the B17S3 milestone Mar 5, 2024
@srl295 srl295 modified the milestones: B17S3, B17S4 Mar 5, 2024
@mcdurdin
Copy link
Member

mcdurdin commented Mar 5, 2024

To my mind, the fatal error is something that is a bug in our compiler. We want to know about it, which is what is happening. It's kinda hard to write a test for this because if we knew what was causing the fatal error, we should be fixing the fatal error instead

@srl295
Copy link
Member Author

srl295 commented Mar 5, 2024

To my mind, the fatal error is something that is a bug in our compiler. We want to know about it, which is what is happening. It's kinda hard to write a test for this because if we knew what was causing the fatal error, we should be fixing the fatal error instead

Fixing the fatal error might require restructuring callbacks and is discussed in #10894

@mcdurdin
Copy link
Member

mcdurdin commented Mar 5, 2024

Fixing the fatal error might require restructuring callbacks and is discussed in #10894

I'm not seeing the restructure discussion?

Base automatically changed from chore/developer/10943-loose-err-match to beta March 6, 2024 04:16
@srl295
fix(developer): remove extra fatal exception
e77a495 
- remove the fatal exception about a failed section compiler,
as it will always otherwise be propagated to the user

Fixes: #10894
@srl295 srl295 force-pushed the fix/developer/10894-expect-the-unexpected branch from b7c7f48 to e77a495 Compare March 8, 2024 15:03
@srl295 srl295 marked this pull request as ready for review March 12, 2024 02:47
@srl295 srl295 modified the milestones: B17S4, B17S3 Mar 12, 2024
@srl295 srl295 changed the title chore(developer): tests for excessive-fatal-exception issue chore(developer): resolve excessive-fatal-exception issue Mar 12, 2024
mcdurdin
mcdurdin approved these changes Mar 12, 2024
View reviewed changes
Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

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

LGTM

@mcdurdin
Copy link
Member

Does this close the original issue?

@srl295 srl295 linked an issue Mar 12, 2024 that may be closed by this pull request
bug(developer): tran failed unexpectedly #10894
Closed
@srl295 srl295 merged commit b575cad into beta Mar 12, 2024
4 checks passed
@srl295 srl295 deleted the fix/developer/10894-expect-the-unexpected branch March 12, 2024 11:35
@keyman-server
Copy link
Collaborator

Changes in this pull request will be available for download in Keyman version 17.0.287-beta

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

@mcdurdin mcdurdin Awaiting requested review from mcdurdin

@darcywong00 darcywong00 Awaiting requested review from darcywong00

Assignees

@srl295 srl295

Labels
chore developer/compilers/ developer/ work-in-progress
Projects
None yet
Milestone
B17S3
Development

Successfully merging this pull request may close these issues.

bug(developer): tran failed unexpectedly
3 participants
@srl295 @mcdurdin @keyman-server