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

runtime error handling #379

Merged
merged 32 commits into from
Jun 27, 2024
Merged

runtime error handling #379

merged 32 commits into from
Jun 27, 2024

Conversation

csgui
Copy link
Collaborator

@csgui csgui commented Apr 11, 2024

Implements #104

Runtime errors have uncovered issues that are being tracked. Some related tests are currently being ignored.

To simplify this PR review, all necessary changes to address these issues will be handled in a separate work after this PR is merged.

@csgui csgui marked this pull request as draft April 11, 2024 16:52
@csgui csgui changed the title runtime error handling [in progress] runtime error handling Apr 11, 2024
@csgui csgui force-pushed the feat/runtime-error-handling branch 5 times, most recently from c93895b to 0b7ea52 Compare April 15, 2024 20:33
Copy link

codecov bot commented Apr 17, 2024

Codecov Report

Attention: Patch coverage is 90.95477% with 36 lines in your changes missing coverage. Please review.

Project coverage is 86.19%. Comparing base (54964f1) to head (c9caf89).

Files Patch % Lines
clar2wasm/src/error_mapping.rs 76.92% 17 Missing and 1 partial ⚠️
clar2wasm/src/tools.rs 60.86% 9 Missing ⚠️
clar2wasm/src/words/conditionals.rs 18.18% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #379      +/-   ##
==========================================
- Coverage   86.26%   86.19%   -0.08%     
==========================================
  Files          43       44       +1     
  Lines       19316    19406      +90     
  Branches    19316    19406      +90     
==========================================
+ Hits        16663    16727      +64     
- Misses       1182     1214      +32     
+ Partials     1471     1465       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@csgui csgui force-pushed the feat/runtime-error-handling branch 2 times, most recently from 84e00dc to 84467d9 Compare April 20, 2024 16:22
@csgui csgui changed the title [in progress] runtime error handling runtime error handling Apr 23, 2024
@csgui csgui marked this pull request as ready for review April 23, 2024 07:56
@csgui csgui requested review from Acaccia, obycode and krl April 23, 2024 07:56
Copy link
Collaborator

@krl krl left a comment

Choose a reason for hiding this comment

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

The code changes look good, some changes suggested.

Also, i have been working on converting the lib_test tests to crosscheck tests in the new style, without macros. Is it necessary to use this test method for some reason or could we convert them to the new style?

clar2wasm/src/initialize.rs Outdated Show resolved Hide resolved
clar2wasm/src/words/control_flow.rs Outdated Show resolved Hide resolved
@csgui csgui requested a review from krl April 24, 2024 11:19
@csgui
Copy link
Collaborator Author

csgui commented Apr 24, 2024

@krl Regarding your question about lib_tests, I see no problem in removing the macros. What is need there is the ability to validate a snippet of Clarity code.

Copy link
Collaborator

@krl krl left a comment

Choose a reason for hiding this comment

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

I think merging anything that ignores more than a few tests is not a good idea.

The reason for using anonymous errors in the first place was because they use different error enums, unifying them first would be better, if we're going this route.

@csgui csgui force-pushed the feat/runtime-error-handling branch from 84e3959 to 177b751 Compare April 30, 2024 08:57
@csgui csgui mentioned this pull request Jun 3, 2024
@csgui csgui force-pushed the feat/runtime-error-handling branch 3 times, most recently from 2c0b250 to dc3a132 Compare June 10, 2024 10:23
@csgui
Copy link
Collaborator Author

csgui commented Jun 10, 2024

@krl tests are not being ignored anymore. Instead, a generic error assertion is in place, which can be changed to a more specific one once compile errors are handled. (#421).

@csgui csgui requested a review from krl June 10, 2024 10:48
clar2wasm/src/error_mapping.rs Show resolved Hide resolved
clar2wasm/src/words/bindings.rs Outdated Show resolved Hide resolved
@smcclellan smcclellan requested a review from krl June 13, 2024 14:04
clar2wasm/src/error_mapping.rs Outdated Show resolved Hide resolved
clar2wasm/src/initialize.rs Outdated Show resolved Hide resolved
clar2wasm/src/standard/standard.wat Outdated Show resolved Hide resolved
clar2wasm/src/wasm_utils.rs Outdated Show resolved Hide resolved
clar2wasm/src/tools.rs Show resolved Hide resolved
clar2wasm/src/words/conditionals.rs Show resolved Hide resolved
clar2wasm/src/words/functions.rs Outdated Show resolved Hide resolved
clar2wasm/src/words/functions.rs Outdated Show resolved Hide resolved
clar2wasm/src/words/functions.rs Outdated Show resolved Hide resolved
clar2wasm/src/words/maps.rs Show resolved Hide resolved
@csgui csgui requested a review from Acaccia June 24, 2024 12:56
@csgui csgui requested a review from Acaccia June 25, 2024 11:18
@csgui csgui requested a review from Acaccia June 25, 2024 17:22
@csgui csgui requested a review from Acaccia June 26, 2024 15:13
Acaccia
Acaccia previously approved these changes Jun 26, 2024
Copy link
Collaborator

@Acaccia Acaccia left a comment

Choose a reason for hiding this comment

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

Great work here!

@csgui csgui enabled auto-merge June 26, 2024 17:45
@csgui csgui added this pull request to the merge queue Jun 27, 2024
Merged via the queue into main with commit 47bf052 Jun 27, 2024
9 checks passed
@csgui csgui deleted the feat/runtime-error-handling branch June 27, 2024 12:45
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.

4 participants