cargo-public-api
cargo-public-api copied to clipboard
Enselic
Deny all the things!
In #72 the foundation for denying API diffs was implemented.
This PR is the next step to allow only certain changes to APIs via CLI parameter.
Tell me what you think, whether this looks good to you. If it does, I can of course add tests!
Thank you for making a follow-up PR!
You have real bad luck with CI because you yet again hit a failure not caused by your changes.
If you rebase on main branch after fetch it should pass.
As for the PR itself, some high level comments:
- The terminology we should use is added, changed, removed (not deleted). I might have wrongly use "deleted" myself in places, but it should be "removed".
- I'm thinking the CLI should look like this:
--deny=added--deny=removed--deny=changed. Maybe even allow combinations such like this:--deny=changed,removed, but feel free to wait with that. - Before a CR I would like tests to be in place and pass in CI, because that indicates significant maturity of the code, and I prefer to review mature code :)
Rebased, fixed your suggestions and also added tests. I'm not 100% sure about whether the tests are correct though, so you should probably have a look at them first :laughing:
Just a tip in case you didn't know: You can run ./scripts/run-ci-locally.sh to see if CI will pass. 99% of the time the script is accurate. I would say 100% for code like in this PR. So you don't have to push code here to see if CI will pass :)
Here's another heads-up: I just merged https://github.com/Enselic/cargo-public-api/pull/90.
It does not cause any merge conflict with this PR (I did a test-merge), but I recommend you rebase on main again to not accidentally start introducing conflicts in your own code.
Rebase was a cake :cake: and I took the liberty to squash away my fixups while I was at it.
Do you have any pointers what I need to fix to get the CI green? I am not seeing it right now :see_no_evil:
(The PR title is used in the autogenerated release-notes, so I changed it to be more descriptive)
I just saw that CI passes! Nice job! Maybe we should land this PR as-is (after review), and then I could look into doing my suggested improvements on top of that (but you are also more than welcomed to take a shot if you are interested)
Let me know your preference
I am a big fan of "get it right the first time", so I am happy to continue iterating on this!
WHOOOP!
CI green! Can I ask you to spend a little portion of your Sunday for a review? If everything is fine, I will do a full history cleanup and then we can do a final round before we merge this - or, of course, if you find anything that can be improved, reiterate (I'm of course more than happy to)!
Your latest suggestions were added in this rebase+squash. I am ready to merge here! :tada:
CI fails because rustdoc JSON format changed in nightly as I mentioned 😅
I'll take care of updating rustdoc JSON and then rebase this and get it merged today 👍
Is it ok if I do it? I'll keep your commits and just add a commit to update readme and --help text
Okay, if that suits your workflow better, feel free! :tada: :+1: :smile: