Skip to content

Conversation

@martinvuyk
Copy link
Contributor

Add Error.as_string_slice() and polish

@martinvuyk martinvuyk requested a review from a team as a code owner April 6, 2025 19:25
@martinvuyk martinvuyk marked this pull request as draft April 6, 2025 19:29
@martinvuyk martinvuyk marked this pull request as ready for review April 6, 2025 20:05
),
")",
)
return String("Error(", repr(self.as_string_slice()), ")")
Copy link
Contributor

@soraros soraros Apr 6, 2025

Choose a reason for hiding this comment

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

Is the repr necessary (can we make it "Error('" etc.)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also don't like building a String unnecessarily, but I don't know whether to remove this. It adds " to the end and beginning of the message or ' if the message contains ", it also encodes control characters in the ASCII range. Since it encodes those characters instead of printing, I think it's worth it in cases where you would want to print something that might have been corrupted on the way. But I'm not sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Maybe we should make repr return a different Writable than String in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe once we have some API for streaming a repr i.e. __repr__(self, mut writer: Writer)

@martinvuyk martinvuyk force-pushed the add-error-as-stringslice branch from 92a9cad to b97aded Compare April 15, 2025 15:40
@laszlokindrat laszlokindrat self-assigned this May 2, 2025
Copy link
Contributor

@laszlokindrat laszlokindrat left a comment

Choose a reason for hiding this comment

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

Thanks! Could you please fix the conflicts so I can bring this in?

@martinvuyk martinvuyk force-pushed the add-error-as-stringslice branch from b97aded to 263b19f Compare May 2, 2025 18:21
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
@martinvuyk martinvuyk force-pushed the add-error-as-stringslice branch from 263b19f to e747213 Compare May 2, 2025 18:21
@martinvuyk
Copy link
Contributor Author

@laszlokindrat done :)

@laszlokindrat
Copy link
Contributor

!sync

@modularbot modularbot added the imported-internally Signals that a given pull request has been imported internally. label May 7, 2025
@modularbot
Copy link
Collaborator

✅🟣 This contribution has been merged 🟣✅

Your pull request has been merged to the internal upstream Mojo sources. It will be reflected here in the Mojo repository on the main branch during the next Mojo nightly release, typically within the next 24-48 hours.

We use Copybara to merge external contributions, click here to learn more.

@modularbot modularbot added merged-internally Indicates that this pull request has been merged internally merged-externally Merged externally in public mojo repo labels May 7, 2025
@modularbot
Copy link
Collaborator

Landed in bc05a00! Thank you for your contribution 🎉

@modularbot modularbot closed this in bc05a00 May 9, 2025
@martinvuyk martinvuyk deleted the add-error-as-stringslice branch May 9, 2025 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

imported-internally Signals that a given pull request has been imported internally. merged-externally Merged externally in public mojo repo merged-internally Indicates that this pull request has been merged internally

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants