-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[stdlib] add __str__
and __repr__
for Optional
#3061
base: nightly
Are you sure you want to change the base?
Conversation
288fe4d
to
58d9f56
Compare
__str__
and __repr__
for Optional
58d9f56
to
9306141
Compare
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.
Nice addition. I think we can be more efficient here by skipping a temporary allocation of a possibly big string.
stdlib/src/collections/optional.mojo
Outdated
writer: The formatter to write to. | ||
""" | ||
if self: | ||
write_to(writer, repr(self.value())) |
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.
Here repr(self.value())
can be a very big String
. If we know that self.value()
has a format_to
method, would it be possible to use it to write directly to the Formatter
?
You can imagine that self.value()
is a List with 10 000 elements for example.
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.
I like the idea, but wouldn't that require use to generally shift away from RepresentableCollectionElement
in these scenarios to something equivalent to Formattable & CollectionElement
?
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.
I agree with Gabriel that we could make this more efficient, though we can do that in a follow-up PR if you'd like 🙂
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.
Might as well do it here. I'm on vacation at the moment but I can look into it on Monday!
!sync |
It looks like I can't sync this because of the merge conflict. @bgreni could you rebase and then I'll merge this? Thanks! 🙂 |
9306141
to
48f11c9
Compare
Done now, apologies for the delay! |
No worries at all, thank you for fixing the conflict! I'll sync this in now. |
!sync |
Signed-off-by: Brian Grenier <[email protected]>
48f11c9
to
ef67c5e
Compare
Add
format_to
__str__
and__repr__