Improve consistency of constructor names for MutableVec and MutableBTreeMap
allsey87 opened this issue · 4 comments
This is a breaking change for a potential 0.4 release. I would propose the MutableBTreeMap::with_values
and MutableVec::new_with_values
be made consistent or even better, removed and replaced with implementations of From<Vec<T>> for MutableVec<T>
and From<BTreeMap<K,V>> for MutableBTreeMap<K,V>
. In the standard library, the with_
prefix is usually only for specifying capacity or a hasher, but not the values.
I agree that a From<Vec<T>>
impl is a good idea, but I also like having a dedicated method for clarity. It's easier to find in the docs.
This is a very interesting case. According to the official guidelines the correct name is either new_with_values
or from_vec
. I'm not sure which option has priority, and I think either option is reasonable.
The stdlib is actually not following that official convention, because it uses with_capacity
instead of new_with_capacity
.
This is a very interesting case. According to the official guidelines the correct name is either
new_with_values
orfrom_vec
.
That is interesting, perhaps the official guidelines have not been updated? After all, new_with_capacity_and_hasher_in
does start getting a bit on the long side.
I guess adding the From
implementations is not a breaking change, but changing the constructors is. Would you accept a PR for just the From
implementations?
That is interesting, perhaps the official guidelines have not been updated?
The API guidelines are updated regularly (the last change was in June 2022), and there are examples in the stdlib which follow the naming convention (such as open_with_offset
or Box::new_zeroed
or SipHasher::new_with_keys
).
But on the other hand you have things like Box::pin
... constructors are a bit of a mess in Rust.
After all,
new_with_capacity_and_hasher_in
does start getting a bit on the long side.
It does recommend to use the builder pattern for complex constructors. That doesn't apply for Signals, since there's only 2 new
methods, and they're both simple.
Would you accept a PR for just the From implementations?
Of course.
So I encountered a small (unrelated) issue while trying to run the tests:
error: unreachable `pub` item
--> src/signal/mod.rs:2:9
|
2 | pub use self::macros::*;
| --- ^^^^^^^^^^^^
| |
| help: consider restricting its visibility: `pub(crate)`
|
= help: or consider exporting it for use by other crates
note: the lint level is defined here
--> src/lib.rs:4:9
|
4 | #![deny(warnings, missing_debug_implementations, macro_use_extern_crate)]
| ^^^^^^^^
= note: `#[deny(unreachable_pub)]` implied by `#[deny(warnings)]`
Working around this for the moment by commenting out line 4 of lib.rs, however, some of the tests seem to hang:
test signal::eq::test_eq has been running for over 60 seconds
test signal::neq::test_neq has been running for over 60 seconds
Any thoughts? I am using:
$ rustc --version
rustc 1.67.0 (fc594f156 2023-01-24)