Feat/issue 1698 global quiet by dharrigan · Pull Request #1704 · casey/just

dharrigan · 2023-10-17T15:17:29Z

Hi,

Here's a first pass at introducing quiet recipes. I'm happy to take unboard any thoughts you may have. Not quite sure about the name set quiet-recipes, but was struggling to come up with something better (maybe set globally-quiet?) :-)

Tests pass (with my additional ones added).

Thank you for your consideration.

#Fixes 1698

-=david=-

casey · 2023-10-22T03:38:59Z

Thanks for the PR!

Some thoughts:

Let's call the setting quiet, so you can do set quiet.
Let's call the attribute [noquiet]
We could also not have an attribute, and invert the meaning of @ if set quiet is used. So if you set quiet, you can do @recipe:, which would make that recipe not quiet. @` is a bit confusing though, so I'm not convinced either way on this.

dharrigan · 2023-10-22T08:18:52Z

Hi,

Thank you for your helpful feedback. I'll make the changes within the next few days :-)

With regards the inversion of the @. I thought about this and I decided on the principle of least surprise (for the user). They are used to @ to signify that the recipe (or line) should be quiet, so I didn't want to break that by introducing an inverted case. I would rather accrete changes than break expectations. I then thought that by introducing the attribute of [no-quiet-recipe] (now [noquiet]), they could then choose to make it explicit if they want an already demarked recipe of @ to become non-quiet when the global attribute is used.

So, no surprises was my thoughts behind that one :-)

-=david=-

dharrigan · 2023-10-24T06:06:36Z

A question...

You've recommended the attribute to be renamed to noquiet. However, there is the precedent of no-exit-message, with a hyphen between the no and the action. Would it be better to be consisent and call the attribute no-quiet?

Thank you.

casey · 2023-10-24T06:08:12Z

Ah, yeah, good catch. In that case it should be no-quiet.

dharrigan · 2023-12-24T11:18:52Z

Hi!

I was wondering if you have had a chance to consider to merge in this PR? Anything else I can do?

Thank you.

-=david=-

casey

Sorry I was slow getting to this! Check out my review comments.

casey · 2024-01-02T01:04:33Z

README.md

      <td><a href="https://www.gentoo.org">Gentoo Linux</a></td>
      <td><a href="https://wiki.gentoo.org/wiki/Portage">Portage</a></td>
-      <td><a href="https://github.com/gentoo-mirror/guru/tree/master/sys-devel/just">guru/sys-devel/just</a></td>
+      <td><a href="https://github.com/gentoo-mirror/dm9pZCAq/tree/master/sys-devel/just">dm9pZCAq/sys-devel/just</a></td>


It looks like there are some unrelated changes, and merge conflicts in README.md.

casey · 2024-01-02T01:04:52Z

README.md

+| ----------------------------------- | -----------------------------------------------                |
+| `[no-cd]`<sup>1.9.0</sup>           | Don't change directory before executing recipe.                |
+| `[no-exit-message]`<sup>1.7.0</sup> | Don't print an error message if recipe fails.                  |
+| `[no-quiet]`<sup>?</sup>            | Override globally quiet recipes and always echo out the recipe |


We should also add [quiet].

There isn't a [quiet] attribute, just [no-quiet] which overrides the set quiet if that's been set.

casey · 2024-01-02T01:05:29Z

README.md

+In addition, an entire Justfile can be marked as being globally quiet by
+setting `set quiet` in the Justfile. For example:


Suggested change

In addition, an entire Justfile can be marked as being globally quiet by

setting `set quiet` in the Justfile. For example:

All recipes in a Justfile can be made quiet with `set quiet`:

casey · 2024-01-02T01:06:00Z

README.md

+If the recipe has the attribute of `[no-quiet]`, then the setting is
+ignored: For example:


Suggested change

If the recipe has the attribute of `[no-quiet]`, then the setting is

ignored: For example:

If a recipe has`[no-quiet]`, then the setting is ignored:

casey · 2024-01-02T01:06:08Z

README.md


 a: b
  @echo A
+


Suggested change

casey · 2024-01-02T01:06:45Z

src/attribute.rs


  #[test]
-  fn to_str() {
+  fn no_exit_message_to_str() {


Suggested change

fn no_exit_message_to_str() {

fn to_str() {

casey · 2024-01-02T01:07:12Z

src/attribute.rs

    assert_eq!(Attribute::NoExitMessage.to_str(), "no-exit-message");
  }
+
+  #[test]


We can remove this test. The other test is there just to make sure that we're serializing attribute names in kebab-case, but we don't need to test every variant.

casey · 2024-01-02T01:08:01Z

src/recipe.rs

  }

+  fn no_quiet(&self) -> bool {
+    !self.attributes.contains(&Attribute::NoQuiet)


Should this be inverted, i.e.:

Suggested change

!self.attributes.contains(&Attribute::NoQuiet)

self.attributes.contains(&Attribute::NoQuiet)

casey · 2024-01-02T01:14:36Z

src/recipe.rs

      let mut continued = false;
-      let quiet_command = lines.peek().map_or(false, |line| line.is_quiet());
+
+      let quiet_command = self.no_quiet() && context.settings.quiet


This is used to determine if we should strip a leading @ from the line, so we should only do so if it starts with an @:

Suggested change

let quiet_command = self.no_quiet() && context.settings.quiet

let quiet_line = lines.peek().map_or(false, |line| line.is_quiet());

casey · 2024-01-02T01:16:41Z

src/recipe.rs

+        || !((quiet_command ^ self.quiet)
+          || (context.settings.quiet && self.no_quiet())
+          || config.verbosity.quiet())


I think this should be something like:

Suggested change

|| !((quiet_command ^ self.quiet)

|| (context.settings.quiet && self.no_quiet())

|| config.verbosity.quiet())

|| !((quiet_line ^ self.quiet) || (context.settings.quiet && !self.no_quiet()) || config.verbosity.quiet())

dharrigan · 2024-01-03T14:56:48Z

Hi!

All changes asked for applied. When you can spare the time, have a looksee and see what you think :-)

Thank you.

-=david=-

casey

Sorry for letting this sit! This looks good, it just needs integration tests, which can go in tests/quiet.rs. The tests in that file are using the test! macro, which I try to avoid these days, so for examples of how to write tests, check out tests/modules.rs. In particular, all the interactions with the quiet setting, the noquiet attribute, quiet lines, and the @ prefix to both recipes in lines, should be tested.

This commit brings the concept of globally quiet recipes to the Justfile. If the justfile has this setting: set quiet With this (implicitly true) setting, every recipe within the Justfile will be quiet - unless the recipe has the (new) attribute of [no-quiet]. closes casey#1698 -=david=-

dharrigan · 2024-01-12T17:18:33Z

Hi,

Additional integration tests done. I hope this is looking better.

-=david=-

casey

Looks good to me!

casey · 2024-01-12T20:40:13Z

Thanks for the PR! I tweaked it a little bit, simplifying the tests and removing an if statement that would never be executed.

This is easily the worst conditional I've ever seen, amazingly using ||, !, ^, and &&:

      if config.dry_run
        || config.verbosity.loquacious()
        || !((quiet_line ^ self.quiet)
          || (context.settings.quiet && !self.no_quiet())
          || config.verbosity.quiet())
      {

But so be it! Functionality for making things quiet or not quiet has just evolved organically over time, so I guess that's just how it is 😂

dharrigan force-pushed the feat/issue-1698-global-quiet branch from 70ebfec to 01c030d Compare December 24, 2023 11:18

casey requested changes Jan 2, 2024

View reviewed changes

dharrigan force-pushed the feat/issue-1698-global-quiet branch 5 times, most recently from a4b98cd to c2d4d29 Compare January 3, 2024 14:54

casey requested changes Jan 12, 2024

View reviewed changes

dharrigan force-pushed the feat/issue-1698-global-quiet branch from c2d4d29 to 9dcb13a Compare January 12, 2024 16:21

dharrigan force-pushed the feat/issue-1698-global-quiet branch from 9dcb13a to fc22352 Compare January 12, 2024 17:18

casey added 5 commits January 12, 2024 12:24

Simplify tests

e5a3be8

Tweak

49f0589

Tweak

1efdd83

Tweak

a0151d8

Tweak

c01243b

casey approved these changes Jan 12, 2024

View reviewed changes

casey added 2 commits January 12, 2024 12:35

Tweak

75385e6

Tweak

a811ed6

casey enabled auto-merge (squash) January 12, 2024 20:37

casey merged commit 5bbc89b into casey:master Jan 12, 2024

dharrigan deleted the feat/issue-1698-global-quiet branch January 13, 2024 07:14

		In addition, an entire Justfile can be marked as being globally quiet by
		setting `set quiet` in the Justfile. For example:

	In addition, an entire Justfile can be marked as being globally quiet by
	setting `set quiet` in the Justfile. For example:
	All recipes in a Justfile can be made quiet with `set quiet`:

		If the recipe has the attribute of `[no-quiet]`, then the setting is
		ignored: For example:

	If the recipe has the attribute of `[no-quiet]`, then the setting is
	ignored: For example:
	If a recipe has`[no-quiet]`, then the setting is ignored:

	!self.attributes.contains(&Attribute::NoQuiet)
	self.attributes.contains(&Attribute::NoQuiet)

	let quiet_command = self.no_quiet() && context.settings.quiet
	let quiet_line = lines.peek().map_or(false, \|line\| line.is_quiet());

Conversation

dharrigan commented Oct 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

casey commented Oct 22, 2023

Uh oh!

dharrigan commented Oct 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dharrigan commented Oct 24, 2023

Uh oh!

casey commented Oct 24, 2023

Uh oh!

dharrigan commented Dec 24, 2023

Uh oh!

casey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dharrigan commented Jan 3, 2024

Uh oh!

casey left a comment

Choose a reason for hiding this comment

Uh oh!

dharrigan commented Jan 12, 2024

Uh oh!

casey left a comment

Choose a reason for hiding this comment

Uh oh!

casey commented Jan 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dharrigan commented Oct 17, 2023 •

edited

Loading

dharrigan commented Oct 22, 2023 •

edited

Loading