Improve the check public API script #59
Conversation
|
Can't reproduce this test failure locally, even after upgrading to the latest nightly :/. |
|
That is exactly my confusion. Locally this PR is correct but when running on the CI VM we are getting a different sort order. |
tcharding
force-pushed
the
01-26-sort-uniq
branch
from
864aee2 to
d376040
Compare
|
This now mimics what I did in I"ve squashed the whole thing down into a single commit, because I"m hacking on the weekend and need to stop now. Can re split it up on Monday if required. |
|
I think the squashing is fine; I use |
|
But what doesn't seem fine is that it appears you've emptied all the .txt files :). I think you can't use |
| - name: Checkout Toolchain | ||
| uses: dtolnay/rust-toolchain@nightly | ||
| - name: Install cargo-public-api | ||
| run: cargo install --locked cargo-public-api |
There was a problem hiding this comment.
Would be nice to cache this.
Ha! And while doing this PR I was like "hmm seems |
tcharding
force-pushed
the
01-26-sort-uniq
branch
from
d376040 to
efac8a1
Compare
|
I've split the PR up and been a bit more careful. The big patch (last one) is now refactor only. |
tcharding
force-pushed
the
01-26-sort-uniq
branch
2 times, most recently
from
9a6bd5b to
a89611a
Compare
|
Ok, now we are back to the same problem we had days ago. The sort order produced when running the script on the CI VM is different to the sort order produced when running it locally on my machine - go figure? I have checked the version of |
|
Maybe the locale differs? From my |
|
Hot tip! I'll investigate, thanks. |
tcharding
force-pushed
the
01-26-sort-uniq
branch
from
a89611a to
1956ded
Compare
|
Oh my goodness, it works. |
|
Lol. Everything may be a file, a text file even, but after 55 years of UNIX development I guess just there wasn't enough demand for some canonical sort order. 🙄 Can you squash this stuff into a nicer PR :) |
Just move the changes in the last patch to a better place, right? |
|
Another lesson should probably be learned - RTFM ... all the way to the bottom. |
I'm curious when and why that warning was added to my manpage. I'll bet it is a fairly recent addition and I'll bet it doesn't show up in many |
Could you squash it so that we introduce the final script in one commit, run it in another (or in the first, that's fine too), and add CI in another? Right now pretty-much every commit is "fix the script somehow and re-run it". |
Re-write the script using functions. Introduces the configuration of locale also. Run the scipt and include the changes to the api files. Note that there are some additions because the old script has not been run for a while.
We have a script that checks for API changes, the intention is that PRs that change the public API have to include a patch to the `api/foo.txt` files. But we forgot to run it in CI - ouch.
tcharding
force-pushed
the
01-26-sort-uniq
branch
from
1956ded to
4148fd7
Compare
|
Done (even though that makes reviewing the api file changes impossible IMO). |
Because this is an initial run, I had no intention of reviewing the API file changes beyond "does it look like a lot of API surface is there and can I reproduce it". |
|
No sweat, lets get it in. |
|
Heh, locally I get which looks like it's a new nightly thing? I'm surprised CI didn't complain. My nightly is from 2024-01-20. |
|
"Structural equality" BTW is the property, roughly, where two types are equal if you can destructure (match) and eventually land on the same enum variants. It's useful for formal proofs because structural equality is something a prover can reason about, unlike the "user implemented a function called |
|
Oh, ran with a nightly from tonight and now it runs clean. Ok, sure. |
Most importantly - actually run it in CI. Face palm.
Also add missing call to
uniqand function-ize the script.