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

chore: deps update to minor versions and fix did-key upstream dep #48

Merged
merged 1 commit into from
Nov 29, 2022

Conversation

zeeshanlakhani
Copy link
Contributor

@zeeshanlakhani zeeshanlakhani commented Nov 18, 2022

Let our deps float on patch versions and fix did-key upstream dep issues.

@pinkforest
Copy link

Fixes #49

ucan/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Member

@cdata cdata left a comment

Choose a reason for hiding this comment

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

@zeeshanlakhani thanks for this change. Can you share the rationale for restricting the version ranges this much?

At a glance, it seems unnecessary to me; as a library, we should be liberal with version ranges so that applications can restrict versions as it suits their use case.

ucan-key-support/Cargo.toml Show resolved Hide resolved
@zeeshanlakhani
Copy link
Contributor Author

zeeshanlakhani commented Nov 28, 2022

@zeeshanlakhani thanks for this change. Can you share the rationale for restricting the version ranges this much?

At a glance, it seems unnecessary to me; as a library, we should be liberal with version ranges so that applications can restrict versions as it suits their use case.

Oh, I don't think we're being too restrictive. We're essentially letting patch versions float, but you don't want feature/major versions to float because they may contain breaking changes, and sometimes minor versions can cause issues as well. With this at least, if dependabot gets a new minor, we can run it through CI.

@cdata
Copy link
Member

cdata commented Nov 28, 2022

I think I agree @zeeshanlakhani but in semantic versioning, 1.0 floats on patch, not minor. ^1 floats on minor. Am I missing something?

@zeeshanlakhani
Copy link
Contributor Author

@cdata my bad, I was texting over mobile at the doctor's office. Yes, this lets patches float, where as some libs, e.g. tokio/tracing/etc, even hardline on the patch.

@zeeshanlakhani zeeshanlakhani changed the title chore: deps update to minor versions chore: deps update to minor versions and fix did-key upstream dep Nov 28, 2022
@pinkforest
Copy link

pinkforest commented Nov 28, 2022

FWIW - Rust ecosystem has whole lot of SemVer exemption footguns to be aware of -
This is why it is common to restrict at major.minor as a lot of breaking changes are exempted to minor bumps
Luckily anyhow's maintainer is very particular about SemVer - often they are not when e.g. deps are exposed via public API's

Raised #51 to perhaps clarify policy.

Copy link
Member

@cdata cdata left a comment

Choose a reason for hiding this comment

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

This looks fine. Thanks for the insights re: semver in cargo.

As a general rule, we should stay liberal with regards to version ranges. If there are specific dependencies that are known to be "bad" semver citizens, we can deal with those on a case-by-case basis. Similarly, if there are dependencies that are highly sensitive (like crypto primitives), let's consider those candidates for more cautious version range strategies. In all special cases, we should document the rationale with comments.

Thanks for making this update!

@cdata cdata merged commit fdd10ba into main Nov 29, 2022
@cdata cdata deleted the zl/fix-deps branch November 29, 2022 02:47
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