Skip to content

Conversation

@dgw
Copy link
Member

@dgw dgw commented Jan 26, 2023

This argument was already nonfunctional in 7.x anyway, due to #1678 and related logging changes. Seems like no one was using it (since we never received a report/complaint that it was broken).

Instead of this, users who need Sopel to run without printing to the terminal can use the usual shell trick:
command >> /dev/null 2>&1

Closes #2397

Checklist

  • I have read CONTRIBUTING.md
  • I can and do license this contribution under the EFLv2
  • No issues are reported by make qa (runs make quality and make test)
  • I have tested the functionality of the things this change touches

Notes

Labeled as breaking change because it needs to be mentioned in the changelog, even though the actual breakage occurred in 7.0.

@dgw dgw added Documentation Breaking Change Stuff that probably should be mentioned in a migration guide labels Jan 26, 2023
@dgw dgw added this to the 8.0.0 milestone Jan 26, 2023
@dgw dgw requested a review from a team January 26, 2023 17:32
This argument was already nonfunctional in 7.x anyway, due to #1678 and
related logging changes. (Seems like no one was using it, since we never
got a report/complaint that it was broken.)

Instead of this, users who need Sopel to run without printing to the
terminal can use the usual shell trick: command >> /dev/null 2>&1
@dgw dgw force-pushed the cli-remove-dead-quiet-arg branch from 2e3f889 to aceedf5 Compare January 26, 2023 17:37
@dgw
Copy link
Member Author

dgw commented Jan 26, 2023

Fixed. Forgot to modify tests before.

Copy link
Contributor

@SnoopJ SnoopJ left a comment

Choose a reason for hiding this comment

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

Change looks good to me. The related tests look a little weird (especially all that hasattr) like they're effectively testing argparse, but such are the joys of long maintenance tails I guess.

@dgw
Copy link
Member Author

dgw commented Jan 26, 2023

The related tests look a little weird (especially all that hasattr) like they're effectively testing argparse

It's testing that someone remembered to add the expected arguments to the parser for each subcommand—or rather, that no one accidentally deletes one of the expected arguments later. @Exirel is very thorough, at least usually. 😁

@Exirel
Copy link
Contributor

Exirel commented Jan 26, 2023

Change looks good to me. The related tests look a little weird (especially all that hasattr) like they're effectively testing argparse, but such are the joys of long maintenance tails I guess.

They are testing we 1. declare an option that will create an attribute of the right name, and then 2. that the value is what was actually expected, with the goal to ensure we don't break our cli's interface.

If a dev remove an argument, or change the name of the destination, that dev will very quickly know that something they might have not tested could be broken. It's a lot of "maybe", I know, yet it's a lot of very cheap maybe so why not.

@dgw dgw merged commit 9dfada5 into master Feb 14, 2023
@dgw dgw deleted the cli-remove-dead-quiet-arg branch February 14, 2023 02:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Breaking Change Stuff that probably should be mentioned in a migration guide Documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cli: --quiet arg doesn't do anything

4 participants