-
-
Notifications
You must be signed in to change notification settings - Fork 390
hash: Future Proofing #2213
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
hash: Future Proofing #2213
Conversation
Instead of providing a non-additive feature `no_sha1` in the future, if ever dependents want to build, say, a SHA-256 version only, anticipate this today by requiring the feature `sha1`. For now, since this crate is useless without support for SHA-1, omitting the feature `sha1` results in a compile error. But in the future, this error could be removed. Dependents that have `default-features = false` will see breakage, and they will have to adjust to default-features = false features = ["sha1"] to recover.
This will allow adding other variants, such as for SHA-256, without breaking dependents.
I just noticed that how the new feature flag handled here is already well-known in this workspace: Lines 8 to 9 in f38e1c9
|
tl;dr: please see #2218 for the commits to be merged. Thanks a lot for the initiative, it's much appreciated. With that said, all is well, but to properly mark releases (and dependent crates) as breaking, I had to prefix the commit messages. If commits aren't signed, I just edit them myself to avoid another review round. As I couldn't push these back here, #2218 was opened for merging. If you'd like to avoid this in future (and I always feel a little guilty to do that as it changes what the author said without checking with them) you can let me know in the PR description or sign your commits (I won't break signatures). Closing this PR in favour of merging the linked one. |
hash: Future Proofing - rewritten commits of #2213
Thanks for picking up my changes, and for your work to fit them into the shape that you expect in this repo. I have nothing against such minor changes to my commits. Of course, I sadly cannot clearly delineate which kind or extent of changes to my commits I would accept, but this was far and clearly on the "yes, please!" end of the spectrum 😃 In fact I should have checked your preferred commit message format more before opening the PR, and should have adjusted the compile error message after my discovery (see #2213 (comment)). A bit of a rough first contribution to Gitoxide. I promise to do better next time 😊 |
That's great, and that's about as much of a change you could ever expect. In one commit I changed the title to be (even) more descriptive, but that's already a rarity. Usually it's only prefixes.
No worries about it at all. I don't have such expectation at all, and it's common practice here to avoid review rounds in favour of editing on top of the submitted commits. That's fastest and most convenient for everyone involved. |
These two changes are to better anticipate future changes to
gix-hash
. See also #281 (comment).Feature for SHA-1 Support
In a future where dependents might want a version that supports, say only SHA-256, it would be problematic to add a non-additive feature
no_sha1
. Anticipate this today by requiring the featuresha1
. For now, since this crate is useless without support for SHA-1, omitting the featuresha1
results in a compile error. But in the future, e.g. once SHA-256 (or others) are supported, reasonably tested, and optional support for SHA-1 is feasible, this error could be removed.Dependents that have
default-features = false
will see breakage (today, because of the compile error), and they will have to adjust theirCargo.toml
to[dependencies.gix-hash] version = "x.y.z" default-features = false +features = ["sha1"]
to recover.
Relevant reading:
Mark
ObjectId
as non-exhaustiveThis will allow adding other variants, such as for SHA-256, without breaking dependents again in the future. It is a breaking change today. ("The earlier, the better.")
Relevant reading: