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.

Sign up for GitHub

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

Jump to bottom

Add TryWriteable with internal Writeable refactoring #4787

Merged
merged 24 commits into from
Apr 10, 2024
Merged

Add TryWriteable with internal Writeable refactoring #4787

merged 24 commits into from
Apr 10, 2024

Conversation

sffc
Copy link
Member

@sffc sffc commented Apr 9, 2024 •
edited

Fixes #4741

This implements the "simpler version" discussed on 2024-04-09 and replaces the more complex version in #4772.

@sffc sffc mentioned this pull request Apr 9, 2024
Closed
@sffc sffc mentioned this pull request Apr 10, 2024
Closed
@sffc sffc marked this pull request as ready for review April 10, 2024 05:53
@sffc
Copy link
Member Author

sffc commented Apr 10, 2024

This currently implements option 5b in #4741 (comment).

sffc
sffc commented Apr 10, 2024
View reviewed changes
utils/writeable/src/helpers.rs Outdated Show resolved Hide resolved
@sffc sffc changed the title Initial TryWriteable with refactoring Add TryWriteable with internal Writeable refactoring Apr 10, 2024
@sffc sffc requested a review from a team as a code owner April 10, 2024 06:06
@sffc sffc removed the request for review from nordzilla April 10, 2024 06:06
robertbastian
robertbastian reviewed Apr 10, 2024
View reviewed changes
utils/writeable/src/lib.rs Show resolved Hide resolved
utils/writeable/src/helpers.rs Outdated Show resolved Hide resolved
utils/writeable/src/lib.rs Outdated Show resolved Hide resolved
utils/writeable/src/lib.rs Show resolved Hide resolved
utils/writeable/src/try_writeable.rs Outdated Show resolved Hide resolved
utils/writeable/src/try_writeable.rs Outdated Show resolved Hide resolved
utils/writeable/src/try_writeable.rs Outdated Show resolved Hide resolved
utils/writeable/src/try_writeable.rs Outdated Show resolved Hide resolved
utils/writeable/src/try_writeable.rs Outdated Show resolved Hide resolved
utils/writeable/src/try_writeable.rs Outdated Show resolved Hide resolved
@sffc sffc requested a review from robertbastian April 10, 2024 15:37
@sffc
Merge branch 'main' into trywriteable2
127dea6 
Conflicts:
	utils/writeable/src/lib.rs
robertbastian
robertbastian reviewed Apr 10, 2024
View reviewed changes
Copy link
Member

@robertbastian robertbastian left a comment

Choose a reason for hiding this comment

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

.

sffc and others added 4 commits April 10, 2024 10:19
@sffc
Implement option 2
341a559
@sffc @robertbastian
Apply suggestions from code review
2dcd850 
Co-authored-by: Robert Bastian <4706271+robertbastian@users.noreply.github.com>
@sffc
Distribute code from helpers.rs
c6d71ad
@sffc
unwrap_or_else
89a227e
@sffc sffc requested a review from robertbastian April 10, 2024 17:29
robertbastian
robertbastian reviewed Apr 10, 2024
View reviewed changes
utils/writeable/src/lib.rs Show resolved Hide resolved
utils/writeable/src/testing.rs Outdated Show resolved Hide resolved
utils/writeable/src/try_writeable.rs Outdated Show resolved Hide resolved
utils/writeable/src/try_writeable.rs Outdated Show resolved Hide resolved
@sffc @robertbastian
Apply suggestions from code review
d4a9d83 
Co-authored-by: Robert Bastian <4706271+robertbastian@users.noreply.github.com>
@sffc sffc requested a review from robertbastian April 10, 2024 17:58
@sffc sffc merged commit f235ef3 into unicode-org:main Apr 10, 2024
29 checks passed
@sffc sffc deleted the trywriteable2 branch April 10, 2024 21:16
@sffc sffc mentioned this pull request Apr 11, 2024
Merged
sffc added a commit that referenced this pull request Apr 18, 2024
@sffc
Rename write_cmp_bytes to writeable_cmp_bytes (#4795)
c7f2424 
See #4402, #4741, #4787
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robertbastian robertbastian robertbastian approved these changes

@zbraniecki zbraniecki Awaiting requested review from zbraniecki

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Should we have a fallible Writeable?
2 participants
@sffc @robertbastian