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

Remove wildcard re-exports #682

Merged
merged 7 commits into from
Mar 27, 2024

Conversation

tcharding
Copy link
Member

Wildcards make it hard to grep for where stuff comes from, explicit imports and re-exports are ... more explicit.

  • Patch 1: Re-export the context types explicitly.
  • Patch 2: Re-export the key types explicitly.

Fix: #681

@sanket1729
Copy link
Member

ACK modulo CI

@apoelstra
Copy link
Member

Looks like CI is red because of formatting.

@tcharding
Copy link
Member Author

Ran the formatter, no other changes.

@sanket1729
Copy link
Member

Ah, sanitizers again :)

@tcharding
Copy link
Member Author

tcharding commented Mar 26, 2024

Are you ok to merge this with the ASAN fail @apoelstra or do you want me to disable ASAN in this repo as well?

EDIT: I just did #687

@tcharding
Copy link
Member Author

No acks so rebasing to get up to date with master branch.

@apoelstra
Copy link
Member

So, in #656 we added a "check for changes to the public API" script but it looks like we have failed to CI-enforce this. Therefore this PR is unnecessarily hard to check. I think, for one, that you are losing a reexport of crate::context::global at the root.

Can you open a PR to run the API script (there are a bunch of changes even on master) and to CI-check this, then rebase this PR on that?

@tcharding
Copy link
Member Author

Yep sure thing.

The `just` command makes scripts and commands discoverable for new devs
and old devs alike when switching between repos.

Add a justfile copied from bitcoin with changes as required.
Add a command to run the `contrib/check-for-api-changes.sh` script.
Run `just check-api` and commit the changes. We should have never gotten
to this state, upcoming patch will check for changes in CI.
Add a job to run the `contrib/check-for-api-changes.sh` script in CI.
Copy the script from `rust-bitcoin`, also add a `just` command to call
it.
@tcharding
Copy link
Member Author

Now on top of #691

Wildcards make it hard to grep for where stuff comes from, explicit
imports and re-exports are ... more explicit.

Import and re-export explicitly instead of by using wildcards.
Wildcards make it hard to grep for where stuff comes from, explicit
imports and re-exports are ... more explicit.

Re-export the `key` types explicitly.
@apoelstra
Copy link
Member

Awesome :). So much easier to review.

I'm gonna try to ACK/merge this even though both Github and my local tools see it as 7 independent commits (even though 5 are already in master). I verified that the first 5 are exactly the same commits in #691.

apoelstra
apoelstra previously approved these changes Mar 27, 2024
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 0da394e

@tcharding tcharding dismissed apoelstra’s stale review March 27, 2024 19:02

The merge-base changed after approval.

@apoelstra apoelstra merged commit b370f67 into rust-bitcoin:master Mar 27, 2024
21 checks passed
@tcharding tcharding deleted the 03-19-rm-wildcard branch April 1, 2024 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

secp256k1-sys/src/lib.rs uses a wildcard import
3 participants