search

sasa1977 / con_cache

ets based key/value cache with row level isolated writes and ttl support
MIT License
910 stars 71 forks source link

Add ConCache.child_spec/1 #44

Closed scarfacedeb closed 6 years ago

scarfacedeb commented 6 years ago

To follow Elixir 1.5 convention and have simpler setup.

The only issue is that it still requires ttl options to be able to start, but I noticed that you don't want to provide the default value for them (e.g. in #42 discussion). That's why I left it as it is.

soundmonster commented 6 years ago

Perhaps updating readme should be also part of this PR?

sasa1977 commented 6 years ago

Thank you for this pull request. I apologize for not looking at it sooner. Somehow I missed the notification and was not aware of it :(

I definitely agree that we should support child_spec, but I think we should replace start_link/2 with start_link/1. Basically we'd move the second argument (which can only have the mandatory :name option) to the options list. That way we end up with only start_link/1, and now child_spec/1 is straightforward to implement.

What do you think?

sasa1977 commented 6 years ago

Perhaps updating readme should be also part of this PR?

Good point. I agree that the readme should be updated once the new interface is in place.

scarfacedeb commented 6 years ago

@sasa1977 I agree about start_link/1 and it's done!

I've also noticed that there were couple of other internal modules that had been using deprecated Supervisor.Spec module. Given the nature of the PR, I decided to refactor them too.

README should be up to date.

Now there's only one problem (apart from breaking changes 😄 ) – CI build is failing, because Elixir 1.5 changed Registry options into keyword list, instead of plain list. (See: https://github.com/elixir-lang/elixir/pull/6189/files#diff-2ebc3220bfeadc10411f4639669baa90L41)

As a result, CI can't start the application with Elixir 1.4.

In theory, we can revert to Supervisor.Spec use with Registry. Or we can abandon Elixir 1.4.

What do you think?