-
Notifications
You must be signed in to change notification settings - Fork 1
[FIX] Load necessary atoms #11
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
Conversation
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
fermuch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
User description
String.to_existing_atom/1was chosen instead ofString.to_atom/1because we do not have control over the strings being converted, given that they come from a external source (Numscript-WASM), which may lead into a overload on our atoms table. The problem is that we did not load some of the atoms needed in any part of the app, thus failing in some points.The solution was create structs, which not only solves the problem, but improve the legibility as well.
PR Type
enhancement, bug_fix, tests
Description
Introduced
PostingandBalancestructs for better atom management.Updated type specifications to use new structs.
Enhanced tests to validate new struct implementations.
Updated README to reflect new struct usage.
Changes walkthrough 📝
3 files
Refactor to use `Posting` and `Balance` structsImplement `Balance` struct and related functionsImplement `Posting` struct and conversion functions2 files
Minor formatting changesMinor formatting changes4 files
Update tests for `Balance` structCorrect test description for `CheckLog`Add tests for `Posting` structUpdate integration tests for new structs1 files
Update documentation for new struct usage