[BUG] nmrs: inconsistent error handling for "Not Found" cases

[cachebag]

• disconnect_vpn() returns Ok(()) if VPN doesn't exist (idempotent)
• forget() returns Err(NoSavedConnection) if connection doesn't exist
• forget_vpn() returns Err(NoSavedConnection) if VPN doesn't exist

This is pretty inconsistent behavior; some operations are idempotent, others aren't.

We can make all "delete/forget/disconnect" operations idempotent (return Ok() if already in desired state)

OR we make them all return errors consistently. Best to document this too.

[ruthwik-01]

Hey @cachebag I am new and would like to work on this.
Should I make all forget* methods idempotent like disconnect_vpn()

[cachebag]

Hey @cachebag I am new and would like to work on this.

Sure, go ahead.

Should I make all forget* methods idempotent like disconnect_vpn()

Yes, just make sure to update the docstrings to document this behavior clearly. Thinking on this more, I think the inconsistency we have now is almost intentional since disconnect operations typically aim for a desired state ("make sure this is disconnected") while forget operations delete a specific resource ("delete this exact thing").

But making everything idempotent is probably more user friendly. Feel free to open the PR whenever you're ready.

[ruthwik-01]

Okay, on it!

[ruthwik-01]

Hey @cachebag,
I have raised a PR for the bug fix
PR: #200
Please let me know if any changes are needed. Thanks!