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

use built-in 'Default' trait #340

Closed
wants to merge 1 commit into from

Conversation

danieleades
Copy link
Contributor

@danieleades danieleades commented Nov 19, 2022

  • use the built-in Default trait for constructing structs with no arguments
  • use idiomatic new method for constructing structs with arguments

capnp/src/message.rs Outdated Show resolved Hide resolved
@danieleades
Copy link
Contributor Author

i've updated this PR in light of the fact that the other breaking changes have been merged.

@danieleades danieleades marked this pull request as ready for review January 17, 2023 07:28
@danieleades danieleades changed the title [WIP] use built-in 'Default' trait use built-in 'Default' trait Jan 17, 2023
@danieleades danieleades requested a review from dwrensha January 17, 2023 13:39
@danieleades
Copy link
Contributor Author

@dwrensha i've refreshed this PR, and deprecated the existing methods rather than removing them outright

@danieleades danieleades requested a review from laundmo November 29, 2023 15:02
Copy link

codecov bot commented Jan 13, 2024

Codecov Report

Attention: 21 lines in your changes are missing coverage. Please review.

Comparison is base (5609a45) 51.88% compared to head (868c67d) 51.82%.
Report is 2 commits behind head on master.

Files Patch % Lines
capnp/src/private/layout.rs 34.61% 17 Missing ⚠️
capnp/src/message.rs 75.00% 2 Missing ⚠️
capnp-rpc/src/rpc.rs 0.00% 1 Missing ⚠️
capnpc/test/test.rs 98.21% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #340      +/-   ##
==========================================
- Coverage   51.88%   51.82%   -0.06%     
==========================================
  Files          69       69              
  Lines       34113    34116       +3     
==========================================
- Hits        17698    17680      -18     
- Misses      16415    16436      +21     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@danieleades
Copy link
Contributor Author

@dwrensha is this still of interest?

@dwrensha
Copy link
Member

dwrensha commented Jan 13, 2024

I added impl Default for message::Builder<HeapAllocator> in 2018225 because that seems potentially helpful if users are defining their own structs that include a message::Builder and they want to have impl Default on those structs. I don't want to remove message::Builder::new_default() because requiring users to find a trait method seems like a potential source of confusion (particularly in situations where the standard prelude is not included). The other changes here are to internal details and don't seem like an improvement to me.

@dwrensha dwrensha closed this Jan 13, 2024
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