refactor: Use write_padded_string() in a couple additional places (Part 2/X)#100
Merged
Byron merged 1 commit intoByron:mainfrom Feb 19, 2025
Merged
Conversation
Additionally, while in the neighhborhood, add `pub(crate)` to the other text_modifications.rs private helpers These being marked as `pub` is misleading since the `text_modifications` module is not actually externally public, and these functions are never re-exported publicly.
56c35cc to
3ab278b
Compare
3 tasks
Byron
approved these changes
Feb 19, 2025
Owner
Byron
left a comment
There was a problem hiding this comment.
Thanks again for this incredibly mindful PR, it's great to have you here!
I now also realise your involvement in Mojo and just have to say: cool stuff and incredible work!
This was referenced Feb 20, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a follow-up to #98, that uses the
write_padded_string()helper in a handful of places I missed in that PR.Additionally, while in the neighhborhood, add
pub(crate)to the other text_modifications.rs private helpers,as we discussed yesterday 🙂. These being marked as
pubis misleading since thetext_modificationsmodule isnot actually externally public, and these functions are never re-exported publicly.
Because you said:
I tucked the
pub(crate)change into this PR instead of making a dedicated PR just for that small change. I am hoping to keep contributing 🙂, and as thePart 2/Xin the title might suggest, I've got a couple of small changes queued up :).I'll try to balance keeping each individual change as small as possible (to make review feel easy and able to fit into whatever time you might have available), will avoiding PRs that are so small that the overhead of reviewing them and merging doesn't feel worth while. As a maintainer myself over at modular/mojo, I know the pain of reviewing over-large PRs.
But I'm happy to adjust to whatever size/style works best for you. Please don't hesitate to suggest any changes to how I'm hoping to contribute here; my goal is to make it as smooth as possible for the maintainers to review my work here🙂