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

Attention: 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 1 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.

[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?