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

Ban use of print macros in non-test code #749

Merged
merged 2 commits into from
Aug 8, 2024
Merged

Ban use of print macros in non-test code #749

merged 2 commits into from
Aug 8, 2024

Conversation

wfchandler
Copy link
Contributor

With #746 we added the _nopipe variants of the (e)print(ln)! macros to avoid panicking when piping to head(1). However, we do not systematically enforce their use, which will inevitably lead inconsistent usage within the project.

Add clippy lints to ban use of the print macros in all non-test code. Either the _nopipe variants should be used when we don't care if the input is read, or write! when the information is crucial, such as an interactive session.

@wfchandler wfchandler requested a review from ahl July 18, 2024 00:48
@ahl
Copy link
Collaborator

ahl commented Jul 18, 2024

This is what the rust big-brains came up with?

@wfchandler
Copy link
Contributor Author

This is what the rust big-brains came up with?

No, I was just looking at what clippy could do for us before going to the council of elders and saw these lints.

@ahl
Copy link
Collaborator

ahl commented Jul 26, 2024

This is what the rust big-brains came up with?

No, I was just looking at what clippy could do for us before going to the council of elders and saw these lints.

Worth asking the council of elders? The _nopipe suffix strikes me as inelegant.

@ahl
Copy link
Collaborator

ahl commented Aug 6, 2024

Given #774 (comment) should we consider other approaches?

With f472bce (Ignore EPIPE in CLI (#746), 2024-07-17) we added the
`_nopipe` variants of the `(e)print(ln)!` macros to avoid panicking when
piping to head(1). However, we do not systematically enforce their use,
which will inevitably lead inconsistent usage within the project.

Add `clippy` lints to ban use of the print macros in all non-test code.
Either the `_nopipe` variants should be used when we don't care if the
input is read, or `write!` when the information is crucial, such as an
interactive session.
@wfchandler
Copy link
Contributor Author

I brought this to the council of big brains going on vacation. There are a few possibilities, but given our need to support Windows I think the approach in this PR is best.

  1. Reset the SIGPIPE handler using the sigpipe crate. Ripgrep was doing this at one point, and we've done this in oxlog, but on Illumos using Rust < 1.82 this is not handled properly. More importantly, this does not work on non-Unix systems.
  2. Manually update the panic handler to ignore errors containing the string 'broken pipe'. This is the approach taken by uutils, but the consensus was that this is a bit janky.
  3. Don't use the standard print macros at all; the approach taken in this PR. If you feel that the _nopipe macros are too wordy, we could rename them to shadow the std macros. The clippy bug is a bit of a pain, but we're aware of it now and I think it's reasonable to watch for it until the lint is fixed.

@ahl
Copy link
Collaborator

ahl commented Aug 8, 2024

Sounds like options 3 is the best option; and let's stay vigilant in code review given the clippy limitations.

@ahl ahl merged commit 5392554 into main Aug 8, 2024
16 checks passed
@ahl ahl deleted the wc/enforce-nopipe branch August 8, 2024 16:34
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.

2 participants