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

Replace Into with From #2169

Merged
merged 7 commits into from
Aug 3, 2021
Merged

Replace Into with From #2169

merged 7 commits into from
Aug 3, 2021

Conversation

thomaseizinger
Copy link
Contributor

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?

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.
@thomaseizinger thomaseizinger changed the title Use type aliases to shorten types Replace Into with From Jul 31, 2021
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

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].

muxers/yamux/CHANGELOG.md Outdated Show resolved Hide resolved
@mxinden
Copy link
Member

mxinden commented Aug 3, 2021

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?

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

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

misc/multistream-select/CHANGELOG.md Outdated Show resolved Hide resolved
@thomaseizinger
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

mxinden commented Aug 3, 2021

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
Copy link
Contributor Author

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
Copy link
Member

mxinden commented Aug 3, 2021

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.

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Thanks!

@mxinden mxinden merged commit 9d65622 into libp2p:master Aug 3, 2021
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.

2 participants