Skip to content

feat(example): calculator realm #4084

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

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

NicolasMelet
Copy link

@NicolasMelet NicolasMelet commented Apr 9, 2025

This calculator realm implements a binary tree to compute the result of mathematics expressions.

  • Make basic computation possible (+ - / * and decimals)
  • store computation functions in a map as lambda
  • write tests
  • add parenthesis in equation

@Gno2D2
Copy link
Collaborator

Gno2D2 commented Apr 9, 2025

🛠 PR Checks Summary

All Automated Checks passed. ✅

Manual Checks (for Reviewers):
  • IGNORE the bot requirements for this PR (force green CI check)
  • The pull request description provides enough details
Read More

🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers.

✅ Automated Checks (for Contributors):

🟢 Maintainers must be able to edit this pull request (more info)
🟢 Pending initial approval by a review team member, or review from tech-staff

☑️ Contributor Actions:
  1. Fix any issues flagged by automated checks.
  2. Follow the Contributor Checklist to ensure your PR is ready for review.
    • Add new tests, or document why they are unnecessary.
    • Provide clear examples/screenshots, if necessary.
    • Update documentation, if required.
    • Ensure no breaking changes, or include BREAKING CHANGE notes.
    • Link related issues/PRs, where applicable.
☑️ Reviewer Actions:
  1. Complete manual checks for the PR, including the guidelines and additional checks if applicable.
📚 Resources:
Debug
Automated Checks
Maintainers must be able to edit this pull request (more info)

If

🟢 Condition met
└── 🟢 And
    ├── 🟢 The base branch matches this pattern: ^master$
    └── 🟢 The pull request was created from a fork (head branch repo: NicolasMelet/gno)

Then

🟢 Requirement satisfied
└── 🟢 Maintainer can modify this pull request

Pending initial approval by a review team member, or review from tech-staff

If

🟢 Condition met
└── 🟢 And
    ├── 🟢 The base branch matches this pattern: ^master$
    └── 🟢 Not (🔴 Pull request author is a member of the team: tech-staff)

Then

🟢 Requirement satisfied
└── 🟢 If
    ├── 🟢 Condition
    │   └── 🟢 Or
    │       ├── 🟢 At least 1 user(s) of the organization reviewed the pull request (with state "APPROVED")
    │       ├── 🟢 At least 1 user(s) of the team tech-staff reviewed pull request
    │       └── 🔴 This pull request is a draft
    └── 🟢 Then
        └── 🟢 And
            ├── 🟢 Not (🔴 This label is applied to pull request: review/triage-pending)
            └── 🟢 At least 1 user(s) of the team tech-staff reviewed pull request

Manual Checks
**IGNORE** the bot requirements for this PR (force green CI check)

If

🟢 Condition met
└── 🟢 On every pull request

Can be checked by

  • Any user with comment edit permission
The pull request description provides enough details

If

🟢 Condition met
└── 🟢 And
    ├── 🟢 Not (🔴 Pull request author is a member of the team: core-contributors)
    └── 🟢 Not (🔴 Pull request author is user: dependabot[bot])

Can be checked by

  • team core-contributors

@leohhhn leohhhn changed the title feat(exemple): calculator realm feat(example): calculator realm Apr 9, 2025
@leohhhn leohhhn self-requested a review April 9, 2025 09:42
"*": func(left float64, right float64) float64 { return left * right },
"/": func(left float64, right float64) float64 {
if right == 0 {
panic("Division by 0 is forbidden")
Copy link
Author

Choose a reason for hiding this comment

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

The 0 division can only be detected when reading the tree, and I don't see a way of properly handling this case that doesn't involve a weird third parameter for all operation function, or a global variable

Copy link
Contributor

@leohhhn leohhhn left a comment

Choose a reason for hiding this comment

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

left a couple of comments. nice work :)

return out
}

func RenderPassion() string {
Copy link
Contributor

Choose a reason for hiding this comment

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

these can be unexported

Copy link
Author

Choose a reason for hiding this comment

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

7787495
Done !

import "testing"

func TestCounter_Addition(t *testing.T) {

Copy link
Contributor

Choose a reason for hiding this comment

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

remove these newlines :)

Copy link
Author

Choose a reason for hiding this comment

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

9aaa711
Done !

Comment on lines 202 to 210
Result: ` + displayVal + `
---------------
| ` + ufmt.Sprintf("[res](%s)", realmPath) + `| ` + ufmt.Sprintf("[(](%s)", realmPath+":?expression="+expression+"<") + `| ` + ufmt.Sprintf("[)](%s)", realmPath+":?expression="+expression+">") + `| ` + ufmt.Sprintf("[del](%s)", realmPath+":?expression="+removeLast(expression)) + `|
|---|---|---|---|
| ` + ufmt.Sprintf("[7](%s)", realmPath+":?expression="+expression+"7") + `| ` + ufmt.Sprintf("[8](%s)", realmPath+":?expression="+expression+"8") + `| ` + ufmt.Sprintf("[9](%s)", realmPath+":?expression="+expression+"9") + `| ` + ufmt.Sprintf("[+](%s)", realmPath+":?expression="+expression+"p") /* here p replaces + because of how + works in bnormal paths*/ + `|
| ` + ufmt.Sprintf("[4](%s)", realmPath+":?expression="+expression+"4") + `| ` + ufmt.Sprintf("[5](%s)", realmPath+":?expression="+expression+"5") + `| ` + ufmt.Sprintf("[6](%s)", realmPath+":?expression="+expression+"6") + `| ` + ufmt.Sprintf("[-](%s)", realmPath+":?expression="+expression+"-") + `|
| ` + ufmt.Sprintf("[1](%s)", realmPath+":?expression="+expression+"1") + `| ` + ufmt.Sprintf("[2](%s)", realmPath+":?expression="+expression+"2") + `| ` + ufmt.Sprintf("[3](%s)", realmPath+":?expression="+expression+"3") + `| ` + ufmt.Sprintf("[*](%s)", realmPath+":?expression="+expression+"*") + `|
| ` + ufmt.Sprintf("[0](%s)", realmPath+":?expression="+expression+"0") + `| ` + ufmt.Sprintf("[.](%s)", realmPath+":?expression="+expression+".") + `| ` + ufmt.Sprintf("[=](%s)", realmPath+":?expression="+expression+"=") + `| ` + ufmt.Sprintf("[/](%s)", realmPath+":?expression="+expression+"/") + `|
`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Try out a library for this, such as p/moul/md

Copy link
Author

Choose a reason for hiding this comment

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

I have used this library for the mardown, but for the columns it isn't exactly what I need as the Columns() function always put "|||' between each column, which gives another display than simply "|"

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh sorry, i meant p/moul/mdtable :)

Copy link
Author

Choose a reason for hiding this comment

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

e10b516
Done !

Comment on lines 199 to 200
Have you ever wanted to do maths but never actually found a calculator ?
Do I have the realm for you...
Copy link
Contributor

Choose a reason for hiding this comment

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

:D

Copy link
Author

Choose a reason for hiding this comment

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

:D

}

func Render(path string) string {
var sb strings.Builder
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer doing out:= "" simply for readability

Copy link
Author

Choose a reason for hiding this comment

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

ffeae14
Done !

@NicolasMelet NicolasMelet marked this pull request as ready for review April 15, 2025 09:52
@Gno2D2 Gno2D2 added the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Apr 15, 2025
@leohhhn
Copy link
Contributor

leohhhn commented Apr 15, 2025

Also, please check why the CI is failing

Copy link

codecov bot commented Apr 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

@NicolasMelet
Copy link
Author

Also, please check why the CI is failing

Is it not because there need to be a positive review ?

@jefft0
Copy link
Contributor

jefft0 commented Apr 16, 2025

Also, please check why the CI is failing

Is it not because there need to be a positive review ?

Yes, that's right. It's normal for the "Merge Requirements" CI to fail while waiting for a review from a core team member.

Copy link
Member

@notJoon notJoon left a comment

Choose a reason for hiding this comment

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

LGTM

remove: review/triage-pending flag

@notJoon notJoon removed the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Apr 17, 2025
@Kouteki Kouteki removed the request for review from a team April 17, 2025 07:16
@Gno2D2 Gno2D2 requested a review from a team April 17, 2025 08:38
}

const (
specialCharacters = "p-*/."
Copy link
Member

@thehowl thehowl Apr 17, 2025

Choose a reason for hiding this comment

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

why not +? (it should be fine as long as it's url-encoded)

Copy link
Author

Choose a reason for hiding this comment

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

It seems like
query.Get("expression")

removes the '+'

I have tried printing the path given by Render and the expression obtained via query.Get

path : ?expression=4+

expression : 4

Still the solution of using req.String allowed me to use '(' and ')' instead of '<' and '>' !

c34cbf4

Comment on lines 217 to 220
table.Append([]string{md.Link("7", realmPath+":?expression="+expression+"7"), md.Link("8", realmPath+":?expression="+expression+"8"), md.Link("9", realmPath+":?expression="+expression+"9"), md.Link("+", realmPath+":?expression="+expression+"p")})
table.Append([]string{md.Link("4", realmPath+":?expression="+expression+"4"), md.Link("5", realmPath+":?expression="+expression+"5"), md.Link("6", realmPath+":?expression="+expression+"6"), md.Link("-", realmPath+":?expression="+expression+"-")})
table.Append([]string{md.Link("1", realmPath+":?expression="+expression+"1"), md.Link("2", realmPath+":?expression="+expression+"2"), md.Link("3", realmPath+":?expression="+expression+"3"), md.Link("*", realmPath+":?expression="+expression+"*")})
table.Append([]string{md.Link("0", realmPath+":?expression="+expression+"0"), md.Link(".", realmPath+":?expression="+expression+"."), md.Link("=", realmPath+":?expression="+expression+"="), md.Link("/", realmPath+":?expression="+expression+"/")})
Copy link
Member

@thehowl thehowl Apr 17, 2025

Choose a reason for hiding this comment

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

line = make([]string, 0, 3)
for idx, c := range "7894561230.=/" {
	query.Set("expression", expression + c)
	line = append(line, md.Link(string(c), req.String()))
	if len(line) == 3 {
		table.Append(line)
		line = line[:0]
	}
}

Copy link
Author

Choose a reason for hiding this comment

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

I had to change the way line is reset here. I'm not 100% confident in gno or go yet, but it seemed like resetting it this way only reseted the index of the []string like a pointer, making it so that every line had the same few characters (. = / here)
instead I assigned a new array to line
line = []string{}

is this a good way to handle this situation in gno ?

c34cbf4

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.

You can use ( ) and + in query strings if you use realmpath, because its string method will encode the characters and make them not special.

@NicolasMelet NicolasMelet requested a review from thehowl April 18, 2025 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧾 package/realm Tag used for new Realms or Packages.
Projects
Status: No status
Status: In Progress
Development

Successfully merging this pull request may close these issues.

6 participants