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: add to_string to numbers #19119

Merged
merged 5 commits into from
Aug 29, 2024
Merged

feat: add to_string to numbers #19119

merged 5 commits into from
Aug 29, 2024

Conversation

thounyy
Copy link
Contributor

@thounyy thounyy commented Aug 28, 2024

Description

Adds to_string to integer modules (u8->u256)

Test plan

Features tests.


Release notes

unsigned integers now support .to_string() methods, for example 10u8.to_string() is the same as b"10".to_string()

  • Protocol:
  • Nodes (Validators and Full nodes):
  • Indexer:
  • JSON-RPC:
  • GraphQL:
  • CLI:
  • Rust SDK:
  • REST API:

Copy link

vercel bot commented Aug 28, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
sui-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 29, 2024 7:42am
3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
multisig-toolkit ⬜️ Ignored (Inspect) Visit Preview Aug 29, 2024 7:42am
sui-kiosk ⬜️ Ignored (Inspect) Visit Preview Aug 29, 2024 7:42am
sui-typescript-docs ⬜️ Ignored (Inspect) Visit Preview Aug 29, 2024 7:42am

Copy link
Contributor

@damirka damirka left a comment

Choose a reason for hiding this comment

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

Thank you for this! Left a note on avoiding duplicates

@@ -57,6 +59,19 @@ module std::u16 {
std::macros::num_sqrt!<u16, u32>(x, 16)
}

public fun to_string(mut value: u16): String {
Copy link
Contributor

Choose a reason for hiding this comment

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

Make it a macro similar to other functions in uN modules.

Copy link
Contributor

@tnowacki tnowacki left a comment

Choose a reason for hiding this comment

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

Generally looks great, thank you so much!
Let's make a macro as @damirka suggests, and let's add a few more interesting test cases

@thounyy
Copy link
Contributor Author

thounyy commented Aug 29, 2024

fixed, sorry for not paying attention to the new implementation!

Copy link
Contributor

@damirka damirka left a comment

Choose a reason for hiding this comment

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

Looking better! Please, also these two commands:

cargo build -p sui-framework
./scripts/update-all-snapshots.sh

Copy link
Contributor

@damirka damirka left a comment

Choose a reason for hiding this comment

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

LGTM, and all checks are green! Thank you @thounyy for this one!

@damirka damirka merged commit df95820 into MystenLabs:main Aug 29, 2024
43 checks passed
dariorussi added a commit that referenced this pull request Aug 29, 2024
jangid pushed a commit to jangid/sui that referenced this pull request Aug 30, 2024
## Description 

Adds `to_string` to integer modules (`u8`->`u256`)

## Test plan 

Features tests.

---

## Release notes

* unsigned integers now support `.to_string()` methods, for example
`10u8.to_string()` is the same as `b"10".to_string()`

- [x] Protocol: 
- [ ] Nodes (Validators and Full nodes): 
- [ ] Indexer: 
- [ ] JSON-RPC: 
- [ ] GraphQL: 
- [ ] CLI: 
- [ ] Rust SDK:
- [ ] REST API:
suiwombat pushed a commit that referenced this pull request Sep 16, 2024
## Description 

Adds `to_string` to integer modules (`u8`->`u256`)

## Test plan 

Features tests.

---

## Release notes

* unsigned integers now support `.to_string()` methods, for example
`10u8.to_string()` is the same as `b"10".to_string()`

- [x] Protocol: 
- [ ] Nodes (Validators and Full nodes): 
- [ ] Indexer: 
- [ ] JSON-RPC: 
- [ ] GraphQL: 
- [ ] CLI: 
- [ ] Rust SDK:
- [ ] REST API:
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.

3 participants