Remove ModelError in favour of strongly-typed errors

[SabrinaJewson]

Description

• Removed ModelError
• Added ReadTokenCacheError and WriteTokenCacheError
• To support the above two types, added ReadFileError and WriteFileError
• Replaced ClientError::Model with the more explicit and useful ClientError::UpdateTokenCache
• Also removed ClientError::CacheFile, which seems to have been left in by accident

Motivation and Context

Strongly-typed errors are better than weakly typed errors because:

• They make it easier for the user to see exactly what can go wrong through just types
• They give better error messages

For example, printing the backtrace of one of the new error types might give this:

Error: failed to read token from cache

Caused by:
0: failed to read file token_cache.json
1: No such file or directory (os error 2)

Which is extremely helpful to the user.

Dependencies

None.

Type of change

Please delete options that are not relevant.

• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

How has this been tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce.

Please also list any relevant details for your test configuration

• Test A: cargo test --features env-file

[marioortizmanero]

I love the idea, thanks for submitting a PR.

• Why not use #[derive(Error)] instead of implementing them manually, though? We already have the dependency anyway.
• Shouldn't ReadTokenCacheError and etc be a variant in ClientError?

[marioortizmanero]

This change can be avoided by making this error part of ClientError

[SabrinaJewson]

But then all code that deals in ClientErrors would now have one more variant to think about even though it can’t actually be created from most code, and similarly any code that calls this function now has to deal with every variant of ClientError even though only one actually can be created.

I suppose it would technically fit ClientError’s stated purpose to use it here, but the API can be made more strongly typed by not using it I think.

[marioortizmanero]

Fair enough. My reasoning was that if ReadTokenCacheError isn't in ClientError, then if someone used to return a ClientError, they will now have to define their own custom error type for both. But I guess that's not really a huge issue, as most libraries will have their own error anyway?

[marioortizmanero]

Why does this use non_exhaustive, for future-proofing? And why not a tuple struct?

[SabrinaJewson]

Could’ve been a tuple struct I suppose — should I change it?

[marioortizmanero]

I'd say we don't really need to future-proof anything here? Not sure what else we would need to add into the error.

[github-actions]

Message to comment on stale PRs. If none provided, will not mark PRs stale