Replace Into with From

[thomaseizinger]

Unless restricted by orphan rules, implementing From is superior
because it implies Into but leaves the choice to the user, which
one to use. Especially for errors, From is convenient because that
is what ? builds on.

Not sure if this needs a changelog entry?

[mxinden]

Cool, thanks!

Not sure if this needs a changelog entry?

Yes please, as it adds From implementations. Also, please bump the patch version of multistream-select in misc/multistream/Cargo.toml and ./Cargo.toml.

[thomaseizinger]

Not sure if this needs a changelog entry?

Yes please, as it adds From implementations. Also, please bump the patch version of multistream-select in misc/multistream/Cargo.toml and ./Cargo.toml.

Okay will add.

On the topic of version bumps: What is the reasoning to do these together with the actual change and not on release time? For all my projects, I usually track all changes in the changelog under an unversioned Unreleased heading and at the time of a new release, decide based on the changes that were made whether or not it is a major, minor or patch release and update the versions accordingly.

A practical implication of this is that cargo's [patch] feature can be used to depend on unreleased versions because it enforces that the version mentioned in the Cargo.toml matches the one that is brought in via [patch].

[mxinden]

On the topic of version bumps: What is the reasoning to do these together with the actual change and not on release time? For all my projects, I usually track all changes in the changelog under an unversioned Unreleased heading and at the time of a new release, decide based on the changes that were made whether or not it is a major, minor or patch release and update the versions accordingly.

A practical implication of this is that cargo's [patch] feature can be used to depend on unreleased versions because it enforces that the version mentioned in the Cargo.toml matches the one that is brought in via [patch].

A couple of arguments that in my eyes speak for the current strategy:

• I am most familiar with a changeset at review / authoring time, not at release time. With that in mind, it is easier for me to determine whether something is a breaking change or not at review / authoring time.
• Doing things right away (at review / authoring time) instead of delayed at release time prevents loosing track of a change. For example, say that this pull request introduced a breaking change to multistream-select. If we don't bump the versions now, I might forget the change entirely at release time, thus pushing a faulty libp2p-core release. If we bump the version now, cargo publish of libp2p-core fails, given that the new version of multistream-select is not yet uploaded. With the fact in mind that each release cycle roughly contains 20 sub releases, this is a major bug source, e.g. see swarm-derive: Update to v0.24.0 #2136 and 40c4287.

A practical implication of this is that cargo's [patch] feature can be used to depend on unreleased versions because it enforces that the version mentioned in the Cargo.toml matches the one that is brought in via [patch].

Indeed, this is a downside of the current approach, though in my eyes a cost worth paying given the extra safety at release time.

What do you think of the arguments above?

[mxinden]

Missing the Cargo.toml bumps. Other than that, this is good to go.

[thomaseizinger]

On the topic of version bumps: What is the reasoning to do these together with the actual change and not on release time? For all my projects, I usually track all changes in the changelog under an unversioned Unreleased heading and at the time of a new release, decide based on the changes that were made whether or not it is a major, minor or patch release and update the versions accordingly.
A practical implication of this is that cargo's [patch] feature can be used to depend on unreleased versions because it enforces that the version mentioned in the Cargo.toml matches the one that is brought in via [patch].

A couple of arguments that in my eyes speak for the current strategy:

• I am most familiar with a changeset at review / authoring time, not at release time. With that in mind, it is easier for me to determine whether something is a breaking change or not at review / authoring time.

• Doing things right away (at review / authoring time) instead of delayed at release time prevents loosing track of a change. For example, say that this pull request introduced a breaking change to multistream-select. If we don't bump the versions now, I might forget the change entirely at release time, thus pushing a faulty libp2p-core release. If we bump the version now, cargo publish of libp2p-core fails, given that the new version of multistream-select is not yet uploaded. With the fact in mind that each release cycle roughly contains 20 sub releases, this is a major bug source, e.g. see swarm-derive: Update to v0.24.0 #2136 and 40c4287.

That does make sense! I am not particularly familiar with releasing within workspaces that have lots of crates, so I can see how this can be an issue.

[thomaseizinger]

Cool, thanks!

Not sure if this needs a changelog entry?

Yes please, as it adds From implementations. Also, please bump the patch version of multistream-select in misc/multistream/Cargo.toml and ./Cargo.toml.

I wasn't quite sure what you are referring to with ./Cargo.toml as multistream-select is not mentioned in there. I bumped the version in misc/multistream-select/Cargo.toml.

[mxinden]

Cool, thanks!

Not sure if this needs a changelog entry?

Yes please, as it adds From implementations. Also, please bump the patch version of multistream-select in misc/multistream/Cargo.toml and ./Cargo.toml.

I wasn't quite sure what you are referring to with ./Cargo.toml as multistream-select is not mentioned in there. I bumped the version in misc/multistream-select/Cargo.toml.

Ah, sorry. Please bump the version in core/Cargo.toml. My bad.

[thomaseizinger]

Cool, thanks!

Not sure if this needs a changelog entry?

Yes please, as it adds From implementations. Also, please bump the patch version of multistream-select in misc/multistream/Cargo.toml and ./Cargo.toml.

I wasn't quite sure what you are referring to with ./Cargo.toml as multistream-select is not mentioned in there. I bumped the version in misc/multistream-select/Cargo.toml.

Ah, sorry. Please bump the version in core/Cargo.toml. My bad.

Shouldn't be necessary, core/Cargo.toml only specifies its dependency using a minor version and as far as I know, cargo by default uses ^ as the semver matcher which takes the highest available patch version.

[mxinden]

Shouldn't be necessary, core/Cargo.toml only specifies its dependency using a minor version and as far as I know, cargo by default uses ^ as the semver matcher which takes the highest available patch version.

Correct. It is a reminder for me to make sure I release multistream-select with the next libp2p release. That said, it is more of a hack, especially as it unnecessarily restricts the number of versions compatible with libp2p-core and thus libp2p.

Let's leave it as is.

[mxinden]

Thanks!