unwatch no longer logs a warning and idempotent behavior clarified

[maximlt]

Resolves #1003

By raising ValueError when trying to remove an already removed/never registered watcher.

[codecov]

Codecov Report

❌ Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 87.04%. Comparing base (274632a) to head (016b675).
⚠️ Report is 26 commits behind head on main.

Files with missing lines	Patch %	Lines
param/parameterized.py	88.88%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1018      +/-   ##
==========================================
- Coverage   87.27%   87.04%   -0.23%     
==========================================
  Files           9        9              
  Lines        4936     4941       +5     
==========================================
- Hits         4308     4301       -7     
- Misses        628      640      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[maximlt]

@jlstevens I believe you're the last person who refactored unwatch, do you mind reviewing this change :) ?

[jlstevens]

After thinking about this, I think raising a ValueError at this stage is too abrupt a change. Instead I propose the following:

1. Improve the warning to use the official warnings module instead of self_.warning
2. If we agree the ValueError is a better approach, update this warning to tell users that in future the warning will become an exception.
3. Change it to a ValueError.

I believe the original intent was to make unwatching idempotent: if you ask for something to be unwatched and it exits, it gets unwatched. If it is not being watched already, then there is nothing to do.

I see arguments either way and I don't mind this behavior being deprecated, but not this suddenly.

[maximlt]

Thanks for the review @jlstevens, beneficial insights. After giving it some more thought, I decided it was best to stay closer to the current implementation that aims to be idempotent. I say "aim" as it is not truly idempotent imo as it logs a warning when the watcher has already been removed. I agree there are situations in an app, even more so when it's async based, where unwatching can happen multiple times or in a non-specific order. unwatch being idempotent makes it so users don't have to wrap all their unwatch calls in a try/except.

FWIW what also pushed me to change things was that the try/except was just catching Exception which can swallow the wrong error if there's a bug in the code.

With 016b675:

• unwatch no longer raises an error or logs a warning; internally in _register_watcher a ValueError is caught and swallowed when a 'remove' action fails
• the docs were updated to declare this behavior (I preferred using no-op instead of idempotent)

@jlstevens could you please review again?

[jlstevens]

Seems you came round to my original way of thinking after all :-)

016b675 looks fine to me: looks like the difference is that there is no more warning. I think I am fine with this: you can now sprinkle repeated unwatch calls as much as you like without annoying messages.

The original thought about the logging is that a well designed app would keep track of what is being watched/unwatched carefully and make sure to pair watch/unwatch properly, issuing a warning to the user if unwatch was called more times than expected (i.e. the watcher was already removed).

However, by declaring unwatch fully idempotent in the documentation (as you have), we can be less opinionated about such things and declare that semantically it is perfectly fine to redundantly call unwatch as many times as you want. Of course people might forget to call unwatch even once but that case was never detected anyway!

Lastly, note that while I think this is fine, this is my somewhat subjective take on the issue. Maybe other people hate this behaviour but my instinct is that this is fairly sensible!