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

Public api #553

Merged
merged 3 commits into from
Sep 24, 2024
Merged

Public api #553

merged 3 commits into from
Sep 24, 2024

Conversation

jwiesler
Copy link
Collaborator

@jwiesler jwiesler commented May 10, 2024

Observations:

  • I omitted auto derived impls and blanket impls, which brought down the api json file from 25mb to 7mb
  • cargo public-api diff does not work, I couldn't get features to be passed to diff (opened an issue for this)
  • The test workflow would work, however, the output is not diffed and the file is 7mb. So this file would have to be diffed manually and the pipeline would always just report "not the same". Maybe there is some nice diff tool that can be used to generate a diff in the pipeline.

Open questions:

  • Does public-api exposes some api to generate the diff programatically?
  • Which features do we want to use to check this?
    • With diff we could check the whole matrix
    • With the test approach every combination of features will need a dedicated file which adds up quickly in repo size

Update:
cargo public-api diff should work after they release a new version on crates.io. This is also what I would use. However, how would we use this? As an optional check just to at least see that something changed?

@jwiesler jwiesler force-pushed the public-api branch 2 times, most recently from 7477a78 to a87cc39 Compare May 10, 2024 15:32
@jwiesler jwiesler force-pushed the public-api branch 8 times, most recently from 989167f to a6a8aa2 Compare May 30, 2024 12:46
@arlyon
Copy link
Owner

arlyon commented May 31, 2024

Hi! It would be handy if we could get a comment on each PR with the changes. It would really help during the weekly codegen changes to see what is different (and by extension the changelogs :))

@jwiesler jwiesler marked this pull request as draft May 31, 2024 16:47
@jwiesler
Copy link
Collaborator Author

This is a proof of concept for cargo public api, we need to talk about what to integrate (I'd favor the job instead of the test). It'd also be great if the new client features would be additive insteadof exclusive such that we can use one tests run, one clippy run and one public-api job run instead of n.

@arlyon
Copy link
Owner

arlyon commented Jul 23, 2024

Will take a proper look at this tonight and come back with some detailed comments. Thanks!

@arlyon
Copy link
Owner

arlyon commented Jul 25, 2024

Nice, I can see the PR is closed out so I will see if I can get this working. I have a CI step that produces unpublished APIs, we can reuse that to post a comment on PRs

@arlyon arlyon force-pushed the public-api branch 3 times, most recently from 30e4438 to 727d0d2 Compare July 25, 2024 09:32
Copy link

Removed items from the public API

(none)

Changed items in the public API

(none)

Added items to the public API

(none)

@arlyon arlyon marked this pull request as ready for review July 25, 2024 09:35
@arlyon
Copy link
Owner

arlyon commented Jul 25, 2024

^ nice! shall we get rid of the test and just use this @jwiesler?

@jwiesler
Copy link
Collaborator Author

jwiesler commented Aug 9, 2024

Sure! I'll just do it right now.

@jwiesler
Copy link
Collaborator Author

jwiesler commented Aug 9, 2024

@arlyon as far as I remember this has one problem: The ci token of actions that run from a commiter that is not a collaborator has severly reduced permissions and can't generate/edit comments.

Edit: see https://stackoverflow.com/a/71683208

@jwiesler jwiesler force-pushed the public-api branch 2 times, most recently from 332f9ca to 958ca4b Compare August 9, 2024 09:03
@jwiesler
Copy link
Collaborator Author

jwiesler commented Aug 9, 2024

I fixed it. Can't merge it though since tokio-hyper doesn't compile on the current nightly (seems to me)

@arlyon arlyon merged commit bf68a94 into master Sep 24, 2024
18 checks passed
@arlyon
Copy link
Owner

arlyon commented Sep 26, 2024

🎉 This PR is included in version 0.40.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants