Conversation
Both Andrew and myself think this was an improvement but Kix won't stop whinging about it. Just remove the whole thing.
82174a3 to
27d1fb9
Compare
|
lol 36,000 lines of red in the diff - irrespective of any other considerations that is a win. |
|
Feel free to postpone this until an alternative is ready. Though it might make me reluctant to make PRs while the check is in. |
|
Sure, but I still don't know what an alternative would look like. I do not want anything that is github-only. We could have a bot which edits PRs for people, but that will cause problems when they try to add commits and find that the upstream version of their PR has extra bot stuff on it. So then we're back to making them run a local tool. We could vendor cargo-check-api into this repo itself? |
If you publicly claim you've reviewed the code and found no backdoors or other stuff you can just publish a git hash. :) |
|
Lets back up, what we have today is (please correct me if I'm wrong):
Would the following keep all the benefits and remove all the negatives:
This would allow:
And finally, when we 1.0.0 a crate add some way of using the tool to check we don't accidentally merge API breaking changes. |
|
@tcharding ok, yeah, that looks like a good summary. And yes, if there were a local tool that would just show the API diff for individual commits (and maybe whole PRs) that would be great. |
|
Leave it with me. |
|
#2946 does the removal, leaving this in draft to remind me to write the tool to use locally. |
eeae225 ci: add cargo-semver-checks (Jose Storopoli) d9567b0 ci: remove check-api (Jose Storopoli) Pull request description: - Removes `check-api` (`cargo-public-api`) - Adds `cargo-semver-checks` ## Note to Reviewers There is a new script `contrib/check-semver.sh` that checks for semver breaks against `master`. If it detects a breaking change it will create the `.semver-break` file. If this file exists then we will add an `API break` label and a big comment to the PR saying: > 🚨 API BREAKING CHANGE DETECTED It reproduces the current behavior of `cargo-public-api`: - `bitcoin`: - `all-features` - `no-default-features` - `base58ck`: - `all-features` - `no-default-features` - `bitcoin_hashes`: - `no-default-features` - `features alloc` - `bitcoin-units`: - `no-default-features` - `features alloc` - `bitcoin-io`: - `no-default-features` - `features alloc` Closes #1875. Supersedes #2912. ## Context Our current test to check API breaks using `cargo public-api` is not "automated" per se. It needs contributors to keep tabs on text files with function signatures and other things and forces reviewers to be the _de facto_ API break detector tool. The idea is to use the well-acclaimed and maintained `cargo semver-checks` that will do this automatically in CI. ACKs for top commit: Kixunil: ACK eeae225 tcharding: ACK eeae225 Tree-SHA512: a77d64b4ca6500b80e6f98f9735d17193f8eaa9998e38602f46418f4b9f3b391ffe9b06bfe8e0285fc4350e8214ebd203034693bac858bac31b7d722903933ae
|
Script written, see #2986 |
Both Andrew and myself think this was an improvement but Kix won't stop whinging about it.
Just remove the whole thing.