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

Flip 293: Stringer interface #294

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

RZhang05
Copy link

Flip #293 working towards onflow/cadence#3579.

@SupunS
Copy link
Member

SupunS commented Oct 22, 2024

[Just cross-referencing] There were some concerns raised before, over having a user-defined toString functions for composites:

Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Nice! Adding a built-in interface, implementing it for built-in types that support it, and allowing Cadence developers to implement the interface for their types, makes a lot of sense 👍


## Objective

This flip proposes the addition of a struct interface `StructStringer` which all string-convertible structs will implement. One goal of this FLIP is to simplify the process for representing `AnyStruct` as a `String`. Secondly, a customizable `toString` will be useful for future string formatting such as string interpolation with this interface.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This flip proposes the addition of a struct interface `StructStringer` which all string-convertible structs will implement. One goal of this FLIP is to simplify the process for representing `AnyStruct` as a `String`. Secondly, a customizable `toString` will be useful for future string formatting such as string interpolation with this interface.
This FLIP proposes the addition of a struct interface `StructStringer` to Cadence, which all string-convertible structs will implement. One goal of this FLIP is to simplify the process for representing `AnyStruct` as a `String`. Secondly, a customizable `toString` will be useful for future string formatting such as string interpolation with this interface.

With the proposed addition, the code could be simplified to
```cadence
access(all) fun toString(_ value: AnyStruct): String? {
if let stringerValue = value as ? {StructStringer} {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if let stringerValue = value as ? {StructStringer} {
if let stringerValue = value as? {StructStringer} {

}
}
```
Additionally a conforming `Struct` can be converted to a `String` in the same function which was not previously possible. This can be useful for various string formatting functions.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Additionally a conforming `Struct` can be converted to a `String` in the same function which was not previously possible. This can be useful for various string formatting functions.
Additionally a conforming struct can be converted to a `String` in the same function which was not previously possible. This can be useful for various string formatting functions.


### Alternatives Considered

An alternative is to have `AnyStruct` itself implement a `toString` function. As a native function there is no longer any risk of malicious code and it still solves the first goal. Additionally, it still allows for more powerful string formatting. The downside of this approach is in limiting customizability for developers.
Copy link
Member

Choose a reason for hiding this comment

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

This might be infeasible, given that not all values of AnyStruct are stringable.

view fun toString(): String
}
```

Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a note regarding naming, i.e. why the name of the interface has the prefix Struct

Comment on lines +102 to +103
- [Display trait](https://doc.rust-lang.org/std/fmt/trait.Display.html) in Rust
- `Stringer` in Go
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- [Display trait](https://doc.rust-lang.org/std/fmt/trait.Display.html) in Rust
- `Stringer` in Go
- [`Display` trait](https://doc.rust-lang.org/std/fmt/trait.Display.html) in Rust
- [`Stringer` interface](https://pkg.go.dev/fmt#Stringer) in Go
- [`CustomStringConvertible`](https://developer.apple.com/documentation/swift/customstringconvertible) in Swift

Copy link
Member

@SupunS SupunS left a comment

Choose a reason for hiding this comment

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

👍 from me

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