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

chore: add print_stdout/print_stderr lints to workspace level #1425

Conversation

oleonardolima
Copy link
Contributor

@oleonardolima oleonardolima commented May 5, 2024

potentially fixes #1362

Description

It adds both print_stdout and print_stderr deny level lints on workspace level, but it does allow it on test fns through clippy.toml settings, and explicitly allow it on example code.

Notes to the reviewers

It currently has the setting allowing it on test fns, but open for discussion below.

Changelog notice

  • Add both print_stdout and print_stderr deny level lints on workspace level

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature

@notmandatory
Copy link
Member

This looks like the right way to prevent prints from getting into the code. But do we even want to allow them on tests? The only place I think they should be allowed is in the examples.

@oleonardolima
Copy link
Contributor Author

oleonardolima commented May 6, 2024

This looks like the right way to prevent prints from getting into the code. But do we even want to allow them on tests? The only place I think they should be allowed is in the examples.

I'm fine with it either way, but I don't see a problem with allowing it for tests.

@oleonardolima oleonardolima force-pushed the chore/add-lint-to-forbid-print-stdout branch from 1524524 to 9f69ee5 Compare May 6, 2024 20:31
@oleonardolima oleonardolima marked this pull request as ready for review May 7, 2024 02:03
@oleonardolima oleonardolima force-pushed the chore/add-lint-to-forbid-print-stdout branch from 9f69ee5 to 24c6655 Compare June 5, 2024 17:56
@notmandatory notmandatory added this to the 1.0.0-beta milestone Jun 5, 2024
Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

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

Concept ACK

Just needs a rebase! We got rid of the bdk_persist crate.

@oleonardolima oleonardolima force-pushed the chore/add-lint-to-forbid-print-stdout branch 2 times, most recently from 08a35f3 to 1ca153a Compare August 14, 2024 19:05
Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning this up, I'd just like to remove the println!() from all tests to make cargo test output more readable. If someone really needs to print stuff while debugging tests they can remove or comment out the print lines.

crates/chain/tests/test_local_chain.rs Outdated Show resolved Hide resolved
clippy.toml Outdated Show resolved Hide resolved
@oleonardolima oleonardolima force-pushed the chore/add-lint-to-forbid-print-stdout branch from 1ca153a to 4cd21c1 Compare August 14, 2024 21:26
@oleonardolima oleonardolima force-pushed the chore/add-lint-to-forbid-print-stdout branch from 4ba2a4c to 026ba3a Compare August 15, 2024 14:53
@oleonardolima oleonardolima force-pushed the chore/add-lint-to-forbid-print-stdout branch from 026ba3a to e063ad8 Compare August 15, 2024 15:01
Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

ACK e063ad8

Looks great! much easier to read through the test output with out all the println noise.

@notmandatory notmandatory merged commit a8d52e6 into bitcoindevkit:master Aug 29, 2024
20 checks passed
@oleonardolima oleonardolima deleted the chore/add-lint-to-forbid-print-stdout branch August 29, 2024 17:30
This was referenced Sep 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Update CI to prevent merging any prints to stdout
3 participants