Make ttl options log an error when missing/invalid

[Secretmapper]

Stab at #41

Not sure if it's the cleanest way to write it but I think it's good/clear enough. Though perhaps it will be better to raise an argument error rather than just logging one?

[Secretmapper]

I think making ttl_check the required/main parameter is a good idea conceptually, but in practice I think having to explicitly set both the parameters (or ttl as false) is better as it ensures the cache works. For example I as a user personally expected only ttl needing to be set - thinking that the ttl_check is done every read.

If ttl_check was the only 'required' parameter, I would have probably thought it was the ttl value, and only set that - not expecting I needed to set a ttl to each item manually. Of course this would be solved if I read the documentation but I think it's best to safeguard this.

Also, thanks for the feedback! I'm still new to elixir so these are great. I've pushed a refactor for it.

For the warnings, do you want me to do a Logger.warn and then return {:ok, cache}? I think there's no real {:warn, :reason} idiom in elixir so Logger.warn should work here?

[sasa1977]

but in practice I think having to explicitly set both the parameters (or ttl as false) is better as it ensures the cache works. For example I as a user personally expected only ttl needing to be set - thinking that the ttl_check is done every read.

Good point!

I still think we could make ttl_check main parameter, and in the case where it is set to false, ttl must not be provided (and if it is, then it's an error). However, if ttl_check is set, the ttl should be either a positive integer (default timeout), or explicitly set to nil (no default timeout).

This would likely require some other changes. For example, we might need to move back to Keyword.fetch (to distinguish between no ttl provided vs ttl: nil). Another question is should we allow storing items which never expire if no default ttl is provided.

So I say let's leave that for now, and if you're interesting in taking this further, open up another issue where we can discuss this.

[Secretmapper]

I think that makes sense! Now that I think about it ttl_check should indeed be the main parameter as it can't be configured otherwise (like ttl can be for each item).

One slight tangent that I think might help is if we adopt the term 'global timeout' or 'cache timeout' instead of 'default timeout'. For example the reason I actually run into #41 was because I thought that there is a default timeout due to the ttl explanation (thought that default expiry for all cache items is 5 seconds instead of being the 'global cache timeout' in the example)

[sasa1977]

It looks like in the recent refactoring we dropped logging, and we lost the distinction between errors and warnings.

Also, you asked me earlier:

For the warnings, do you want me to do a Logger.warn and then return {:ok, cache}? I think there's no real {:warn, :reason} idiom in elixir so Logger.warn should work here?

At this point, I'm starting to think that it's becoming overly complicated to distinguish between errors and warnings, given that we plan to drop warnings anyway. Also, since we agree that ttl_check should become the main parameter, I think it would be simpler to make that change, and error on every invalid input.

So, being aware that I'm changing my mind and expanding the scope (sorry about that), I'm thinking about immediately moving to something like this:

• ttl_check is mandatory
• ttl is mandatory if ttl_check is not nil
• Everything else is an error, including the case where ttl_check == nil and ttl != nil.
• On error, we raise an exception with the descriptive message.
• We should likely rename both parameters. How about ttl_check_inteval and global_ttl?
• We need to update readme, docs, and changelog.

What do you think?

[Secretmapper]

Hello @sasa1977

On error, we raise an exception with the descriptive message.

By this you mean we actually raise an error (raise msg) instead of returning {:error, msg}?

[sasa1977]

By this you mean we actually raise an error (raise msg) instead of returning {:error, msg}?

Yes. I don't see a point in returning {:error, reason} here, since client function can't really fix the problem. An error here means that invalid arguments have been passed, so I think we should raise, just like e.g. String.to_integer(:foo) will raise. I think that raise ArgumentError, error_message would be appropriate here.

What do you think?

[Secretmapper]

That makes sense! I've added the changes to make it raise an error.

[sasa1977]

Thanks!

Head's up that I'm away this week, so not sure if I'll be able to review, but I'll see if I can squeeze it in. In the worst case, I'll do it early next week.

[Secretmapper]

Noted, and no worries!

[sasa1977]

Thank you for contributing, and for your patience! I'd like to do another pass through docs/specs before releasing the new version. My schedule is currently quite full, but I hope I'll be able to do it by the end of the month.