feat(watch): implement --debounce flag with duration by lucascherzer · Pull Request #16187 · nushell/nushell

lucascherzer · 2025-07-16T15:04:01Z

Fixes #16178

Description

This PR implement --debounce flag with duration for watch.

This new flag has a weird issue with calculating the durations.

When I timed it with a debounce time of 1min, it registered the write at 1m11s...

User-Facing Changes

Release notes summary

New `watch --debounce` option

The watch command now has --debounce flag, which takes a duration value. This will replace the --debounce-ms flag which takes an int rather than a duration, and will eventually take over its -d short flag.

Deprecate `watch --debounce-ms`

TODO(release-notes): Move this to Deprecations section
TODO(release-notes): verify link works after generating ToC

With the new watch --debounce option, the --debounce-ms option is no longer necessary. Use watch --debounce with a duration value instead.

Tests + Formatting

🟢 toolkit fmt
🟢 toolkit clippy
🟢 toolkit test
🟢 toolkit test stdlib

After Submitting

Update the online docs for watch:

new flag --debounce: duration
-d is now the shorthand of --debounce
--debounce-ms will be deprecated

This new flag has a weird issue with calculating the durations. When I timed it with a debounce time of 1min, it registered the write at 1m11s...

lucascherzer · 2025-07-16T15:54:42Z

Testing this again after the short change, when I make the debounce time 1min, it registers the write after 1m5s.

lucascherzer · 2025-07-16T16:59:55Z

After investigating this, it seems to be an issue with the crate that does the inode watching itself (notify_debouncer_full).

It does not seem to use hooks for file changes and instead monitors them at regular intervals.
These intervals are defined as 1/4 * debounce_time [1], meaning the 5 second and 11 second delays I noticed were within this check interval of 15sec given the debounce time of 1 minute and thus, to be expected.

[1] https://docs.rs/notify-debouncer-full/latest/notify_debouncer_full/fn.new_debouncer.html

fdncred · 2025-07-16T17:20:32Z

instead of deprecating non-duration times, you could make the signature a SyntaxShape::OneOf(SyntaxShape::Int, SyntaxShape::Duration). This avoids a breaking change while still allowing both scenarios.
If the current crate isn't good enough, we could explore exchanging it for a better one that works better or is better supported, assuming it works cross platform.

lucascherzer · 2025-07-16T17:36:27Z

Regarding 1
we discussed deprecation in #16178. The existing flag is named debounce-ms. If that were to accept both Int and Duration, I would find that very confusing given its suffix.

Regarding 2
In my opinion, it works well enough in this use case. Though we should maybe discuss this in a dedicated issue/discussion. I can imagine there are scenarios where this might not be acceptable for users.

fdncred · 2025-07-16T18:47:29Z

ya, debounce-ms wouldn't make sense for other duration units

sholderbach

Thanks for tackling this so quickly, looks pretty much as expected!

sholderbach · 2025-07-16T18:52:06Z

crates/nu-command/src/filesystem/watch.rs

+                debounce_duration = match u64::try_from(val.item) {
+                    Ok(val) => Duration::from_millis(val),
+                    Err(_) => {
+                        return Err(ShellError::TypeMismatch {
+                            err_message: "Debounce duration is invalid".to_string(),
+                            span: val.span,
+                        });
+                    }
+                }
+            }


Pinging @132ikl on how we now raise the deprecation warning correctly since #16147

The ShellError::DeprecationWarning still exists but I guess that is not the way to go anymore.

if you are referring to the second error, it is actually a type mismatch. This condition occurs when using --debounce-ms with an arg that can not be converted to a u64.

In the Ok case we would want to send the deprecation warning that you use the old style flag. (at least that's how we did handled that so far)

You mean stop execution and exit with the warning? So that the old flag is never actually executed?
I.e.

Ok(_) => return Err(ShellWarning::Deprecated ...)

No still running but reporting the warning before returning the old result. I think that mechanism was revised with #16147 (before you would run report_error or similar with a ShellError::DeprecationWarning.)

Sounds good! I implemented your feedback

yes, this should be handled via a DeprecationEntry. You don't need to do anything special at run-time, just handle it the same way as before (maybe add a comment noting to remove the section of code once we remove the deprecated option)

Here's an example of how you can set this up, you should add a fn deprecation_info to the impl Command for Watch and then use DeprecationType::Flag. Also we generally do two releases out for deprecations, so you can set since to 0.106.0 and expected_removal to 0.108.0. Let me know if you have any issues!

SECURITY.md

sholderbach · 2025-07-17T14:28:23Z

crates/nu-protocol/src/value/mod.rs

        internal_span: Span,
    },
    Duration {
+        /// The duration in nanoseconds.


good catch, wouldn't have guessed that that was not documented here

I was confused initially. Thought it'd be milliseconds

lucascherzer · 2025-07-17T15:45:53Z

crates/nu-command/src/filesystem/watch.rs

@@ -120,7 +121,18 @@ impl Command for Watch {
            }
            (None, Some(val)) => {
                debounce_duration = match u64::try_from(val.item) {


This I was not sure about. If someone can tell me what its supposed to say, I can adjust it. We should then document this as well

good point, I didn't even think to document the new deprecation process. I can add something to devdocs when I get a chance

Is this correct, or do I have to change something here?

lucascherzer

@132ikl is this what you meant?

lucascherzer · 2025-07-17T19:03:32Z

I'll have a look at the failed CI tomorrow

132ikl · 2025-07-17T20:58:31Z

Yes, the deprecation_info looks good. You don't need the report_shell_error stuff when using deprecation_info, feel free to just remove that

132ikl

one small thing, otherwise looks good to me 😄

132ikl · 2025-07-18T14:38:16Z

crates/nu-command/src/filesystem/watch.rs

+            (Some(v), None) => match v.item {
+                dur @ Value::Duration { val, .. } => {
+                    debounce_duration = match u64::try_from(val) {
+                        Ok(d) => Duration::from_nanos(d),
+                        Err(_) => {
+                            return Err(ShellError::TypeMismatch {
+                                err_message: "Debounce duration is invalid".to_string(),
+                                span: dur.span(),
+                            });
+                        }
+                    };
+                }
+                // This should be unreachable, as the parser will fail
+                // before this is ever called. I will leave it in anyways
+                // so we can handle it gracefully once it never arrives
+                any => {


I think we can just do this here:

Suggested change

(Some(v), None) => match v.item {

dur @ Value::Duration { val, .. } => {

debounce_duration = match u64::try_from(val) {

Ok(d) => Duration::from_nanos(d),

Err(_) => {

return Err(ShellError::TypeMismatch {

err_message: "Debounce duration is invalid".to_string(),

span: dur.span(),

});

}

};

}

// This should be unreachable, as the parser will fail

// before this is ever called. I will leave it in anyways

// so we can handle it gracefully once it never arrives

any => {

(Some(v), None) => {

let Value::Duration { val, ..} = v.item else {

return Err(ShellError::TypeMismatch {

err_message: "Debounce duration must be a duration".to_string(),

span: v.span,

});

}

val

this suggestion won't apply cleanly since I can't do a suggestion over unchanged lines, but the any match branch would have to be manually removed as well

fair, that is kind of repetitive. In c92711c, I removed the any branch and refactored this according to your suggestion. I also use v.item.span() instead of v.span due to the deprecation notice

Semantically there is a difference between the Spanned.span returned from a CallExt/Call argument handler and the span inherent to the Value which you get by Value.span() (and is stored in internal_span.

The Spanned<T> should be filled with the span of the particular argument expression while the Value has its span pointing to its point of creation.

While this should be the same for a literal I think this would point to two different locations when you for example pass a variable to an argument

let foo = 42 bar --baz $foo

So do you want this to use v.span instead? What about the deprecation notice - how is this planned to go in the future?

crates/nu-command/src/filesystem/watch.rs

sholderbach · 2025-07-29T22:12:52Z

Can you prepare a paragraph for the upcoming release notes mentioning what you have to going forward. Either by expanding User-facing Changes or directly in the release notes nushell/nushell.github.io#1999

This PR reverts the breaking change of short flag change of `watch -d` to `--debounce` instead of `--debounce-ms`. This fully prevents #16187 from being a breaking change. Before #16187: ```nushell watch -d 1000 foo {} # => Now watching files at "/home/rose/foo". Press ctrl+c to abort. ``` Before this PR (after #16187): ```nushell watch -d 1000 foo {} # => Error: nu::parser::parse_mismatch # => # => × Parse mismatch during operation. # => ╭─[entry #15:1:10] # => 1 │ watch -d 1000 foo {} # => · ──┬─ # => · ╰── expected duration with valid units # => ╰──── ``` After this PR (after #16187): ```nushell watch -d 1000 foo {} # => Warning: nu::parser::deprecated # => # => ⚠ Flag deprecated. # => ╭─[entry #3:1:7] # => 1 │ watch -d 1000 foo {} # => · ─┬ # => · ╰── watch --debounce-ms was deprecated in 0.107.0 and will be removed in 0.109.0. # => ╰──── # => help: `--debounce-ms` will be removed in favour of `--debounce` # => # => Now watching files at "/home/rose/foo". Press ctrl+c to abort. ``` This PR also fixes the `DeprecationEntry` which incorrectly had a `--` in it, which I failed to realize when reviewing #16187. We should add a `debug_assert` or something for this. Rel: #16187 ## Release notes summary - What our users need to know N/A ## Tasks after submitting - [ ] Add `debug_assert` for `DeprecationType::Flag`s with `--`

lucascherzer · 2025-09-02T14:47:43Z

Can you prepare a paragraph for the upcoming release notes mentioning what you have to going forward. Either by expanding User-facing Changes or directly in the release notes nushell/nushell.github.io#1999

Sorry, I overlooked this. Does this still need to be done?

132ikl · 2025-09-02T15:22:36Z

no worries, I added a release notes summary for the PR already

lucascherzer · 2025-09-02T15:44:00Z

thanks :D

lucascherzer added 2 commits July 16, 2025 16:57

feat(watch): implement --debounce flag with duration

d331ec7

This new flag has a weird issue with calculating the durations. When I timed it with a debounce time of 1min, it registered the write at 1m11s...

refactor: use from_nanos instead of dividing and calling from_millis

49acd1c

lucascherzer marked this pull request as ready for review July 16, 2025 16:59

sholderbach reviewed Jul 16, 2025

View reviewed changes

docs: add docstring

66cadf4

sholderbach reviewed Jul 17, 2025

View reviewed changes

lucascherzer and others added 3 commits July 17, 2025 17:37

feat: deprecation warning when using old flag

dacdc1b

Merge branch 'main' into feat/watch-duration

94f8ee3

fix: merge

575a2db

lucascherzer commented Jul 17, 2025

View reviewed changes

feat: implement deprecation_info for Watch

3c848a0

lucascherzer commented Jul 17, 2025

View reviewed changes

fix: remove duplicate deprecation warning

e9227d5

132ikl reviewed Jul 18, 2025

View reviewed changes

lucascherzer added 2 commits July 21, 2025 07:09

refactor: remove any branch and simplify

c92711c

style: refactor according to clippy rules

de66b9e

132ikl approved these changes Jul 29, 2025

View reviewed changes

Bahex requested changes Jul 29, 2025

View reviewed changes

crates/nu-command/src/filesystem/watch.rs Outdated Show resolved Hide resolved

Adjust version numbers for the next release.

f8f049b

Bahex approved these changes Jul 29, 2025

View reviewed changes

Bahex merged commit 459f3c0 into nushell:main Jul 29, 2025
16 checks passed

github-actions bot added this to the v0.107.0 milestone Jul 29, 2025

sholderbach added the category:deprecation Related to the deprecation of commands/features/options label Jul 29, 2025

sholderbach added notes:breaking-changes This PR implies a change affecting users and has to be noted in the release notes notes:mention Include the release notes summary in the "Hall of Fame" section labels Jul 29, 2025

132ikl removed the notes:mention Include the release notes summary in the "Hall of Fame" section label Aug 19, 2025

132ikl mentioned this pull request Aug 19, 2025

Mitigate watch -d breaking change #16473

Merged

1 task

Conversation

lucascherzer commented Jul 16, 2025 • edited by 132ikl Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

User-Facing Changes

Release notes summary

New watch --debounce option

Deprecate watch --debounce-ms

Tests + Formatting

After Submitting

Uh oh!

lucascherzer commented Jul 16, 2025

Uh oh!

lucascherzer commented Jul 16, 2025

Uh oh!

fdncred commented Jul 16, 2025

Uh oh!

lucascherzer commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fdncred commented Jul 16, 2025

Uh oh!

sholderbach 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!

lucascherzer Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

lucascherzer left a comment

Choose a reason for hiding this comment

Uh oh!

lucascherzer commented Jul 17, 2025

Uh oh!

132ikl commented Jul 17, 2025

Uh oh!

132ikl left a comment

Choose a reason for hiding this comment

Uh oh!

132ikl Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucascherzer Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Uh oh!

sholderbach commented Jul 29, 2025

Uh oh!

lucascherzer commented Sep 2, 2025

Uh oh!

lucascherzer commented Jul 16, 2025 •

edited by 132ikl

Loading

New `watch --debounce` option

Deprecate `watch --debounce-ms`

lucascherzer commented Jul 16, 2025 •

edited

Loading

lucascherzer Jul 17, 2025 •

edited

Loading

132ikl Jul 18, 2025 •

edited

Loading

lucascherzer Jul 21, 2025 •

edited

Loading