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

feat(gnolang): add support for octals without 'o' (eg. 0755) #1331

Merged
merged 11 commits into from
Dec 18, 2023

Conversation

notJoon
Copy link
Member

@notJoon notJoon commented Nov 3, 2023

Updated the condition in doOpEval to detect octal literals that start with a '0' followed by octal digits (0-7)

related with: #1322

@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Nov 3, 2023
Copy link

codecov bot commented Nov 3, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1126d9f) 55.98% compared to head (4cfe6fa) 56.48%.
Report is 22 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1331      +/-   ##
==========================================
+ Coverage   55.98%   56.48%   +0.49%     
==========================================
  Files         420      423       +3     
  Lines       65443    66738    +1295     
==========================================
+ Hits        36640    37698    +1058     
- Misses      25940    26142     +202     
- Partials     2863     2898      +35     

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

gnovm/pkg/gnolang/op_eval.go Show resolved Hide resolved
gnovm/pkg/gnolang/op_eval_test.go Outdated Show resolved Hide resolved
@thehowl thehowl changed the title feat: Support Traditional Octal Literals feat(gnolang): add support for octals without 'o' (eg. 0755) Nov 9, 2023
Copy link

netlify bot commented Nov 10, 2023

Deploy Preview for gno-docs2 canceled.

Name Link
🔨 Latest commit fe3c03e
🔍 Latest deploy log https://app.netlify.com/sites/gno-docs2/deploys/6551f12ca7d5c90008f0155c

@notJoon notJoon requested a review from thehowl November 13, 2023 07:44
@notJoon
Copy link
Member Author

notJoon commented Nov 13, 2023

@thehowl It seems I'm ready to go. I've changed the previous test to run directly on the doOpEval method as you mentioned :)

Copy link
Member

@thehowl thehowl left a comment

Choose a reason for hiding this comment

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

LGTM after moving to table test!

gnovm/pkg/gnolang/gno_test.go Outdated Show resolved Hide resolved
@thehowl
Copy link
Member

thehowl commented Nov 30, 2023

On a second thought, please also make sure to add a couple of test to gnovm/tests/files/, on top of the one you already did.

@notJoon
Copy link
Member Author

notJoon commented Dec 1, 2023

LGTM after moving to table test!

I modified them to handle negative expressions (e.g., -0x10101) and to test for multiple base systems, which was not the case previously.

@notJoon notJoon requested a review from a team as a code owner December 1, 2023 02:24
Copy link
Contributor

@jaekwon jaekwon left a comment

Choose a reason for hiding this comment

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

TY!

@jaekwon jaekwon merged commit 1cfe9b4 into gnolang:master Dec 18, 2023
186 checks passed
gfanton pushed a commit to moul/gno that referenced this pull request Jan 18, 2024
…ng#1331)

Updated the condition in `doOpEval` to detect octal literals that start
with a '0' followed by octal digits (0-7)

related with: gnolang#1322
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🤖 gnovm Issues or PRs gnovm related
Projects
Status: Done
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants