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.

Sign up for GitHub

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

Jump to bottom

integration_test: Clear clippy warnings #121

Merged
merged 4 commits into from
Jun 11, 2024
Merged

integration_test: Clear clippy warnings #121

merged 4 commits into from
Jun 11, 2024
+13 −13

Conversation

tcharding
Copy link
Collaborator

@tcharding tcharding commented May 17, 2024 •
edited

Clear all the clippy warnings from the integration_test crate. Done in preparation for re-writing CI to use the new run_task script. The last patch is bodgy.

@tcharding tcharding mentioned this pull request May 17, 2024
Open
@apoelstra
Copy link
Owner

Last patch is interesting. The code in question is unsound. May be worth filing a compiler bug showing that the take code triggers the warning but the replace code does not, because I believe they are exactly equivalent.

Meanwhile, we should replace this unsafe { &mut RESULTS } shit with a mutex. Will need to use a OnceCell or something to initialize it.

@apoelstra
Copy link
Owner

Looks like bitcoincore-rpc, where we copied this from, now just uses lazy_static. We might as well copy the same solution. Or, we can use Mutex::new() in a const context, but only since 1.63, so if we did this, we'd no longer be able to run the integration tests with MSRV.

@apoelstra
Copy link
Owner

I propose:

  • We PR to fix the unsoundness (and nothing else)
  • Then we rebase this PR, and add a commit which whitelists the broken assigning_clones lint.
  • Then we rebase the "rewrite CI" PR, adding whatever we need to pin the nightly version.

Then I think we should get green CI on all three.

@apoelstra apoelstra mentioned this pull request May 17, 2024
Merged
@tcharding
Copy link
Collaborator Author

Thanks for putting some time into this pile of PRs - I'll have a ago at your suggestion week (its the weekend now for me).

@tcharding tcharding mentioned this pull request May 31, 2024
Closed
@tcharding tcharding force-pushed the 05-17-clippy branch 2 times, most recently from 0f46dcd to b45a071 Compare May 31, 2024 01:52
@tcharding tcharding marked this pull request as draft May 31, 2024 01:53
@tcharding tcharding mentioned this pull request May 31, 2024
Draft
This was referenced May 31, 2024
Draft
Open
@tcharding
Remove unnecessary return statement
4f7b9fc 
clippy emits various warnigs of type:

  warning: unneeded `return` statement

As suggested, remove unnecessary return statement.
@tcharding
Remove useless call to into
c766f45 
clippy emits various warnings of form:

  warning: useless conversion to the same type: `&str`

As suggested, remove the call to `into()`.
@tcharding
Remove empty string literal from println
f33351d 
clippy emits various warnings of form:

  warning: empty string literal in `println!`

As suggested, remove the double quotes.
@tcharding
Use unwrap_or_else
ac3fa84 
Clippy emits:

  warning: use of `expect` followed by a function call

As suggested, use `unwrap_or_else` instead.
@tcharding tcharding marked this pull request as ready for review May 31, 2024 19:11
@tcharding
Copy link
Collaborator Author

Bump please.

apoelstra
apoelstra approved these changes Jun 11, 2024
View reviewed changes
Copy link
Owner

@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 ac3fa84

@apoelstra apoelstra merged commit 17dd97a into apoelstra:master Jun 11, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apoelstra apoelstra apoelstra approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@tcharding @apoelstra