Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace dicts with maps (and more) #16

Merged
merged 1 commit into from
Sep 19, 2022

Conversation

Maria-12648430
Copy link
Contributor

This PR is a mashup of several things. I noticed that too late to separate them into several commits 😓

  • All usages of dicts have been replaced with maps (except one, which I'm not sure about).
  • The start_link functions now also accept maps on top of proplists, and the tests have been changed to use the maps variant. The function proplists:to_map/1 used to convert proplists intomaps appeared only as late as OTP 24. For that reason, I backported this function (copied from https://github.com/erlang/otp/blob/master/lib/stdlib/src/proplists.erl#L704-L720) into depcache, to be usable when running on older (pre-24) OTP versions.
  • The configuration given to start_link is now validated, that is, config maps (and by extension, proplists) are not allowed to contain keys other than memory_max and callback, and if present, the values are validated to be either undefined, an integer >= 0 for memory_max, or a tuple of module, function and arguments (see next two points). If a config is given that is by this definition invalid, a badarg error is raised.
  • More/corrected/improved typing/specs. Specifically, some types/specs denoted mfa() - {Module, Function, Arity} - but were actually used in the code as {Module, Function, Arguments}.
  • The value tuple for the callback option can also be given with a single (non-list) value for the arguments, in which case it is reinterpreted to mean a single argument for the given Module:Function. This is ambiguous and therefore questionable, in fact it should be discouraged. The specs provided with this PR still keep that usage open (therefore the no_warn directive for callback/3), but dialyzer will (should) complain if users use the single non-list variant for callback.
  • The tick timer has been changed from an interval timer to a timer that is restarted whenever the tick message has been received. This removes the need to flush missed ticks from the message queue.

@Maria-12648430
Copy link
Contributor Author

Ok, some tests fail, for reasons I didn't see coming XD I'll see to that tomorrow. Or today, it's past midnight already 🥱

* Replace all usages of `dict` with maps
* Fix wrong types and specs
* Validate configuration
@Maria-12648430
Copy link
Contributor Author

Maria-12648430 commented Sep 18, 2022

Previously failing tests should be fixed.

@mworrell
Copy link
Member

You are fast!

@mworrell
Copy link
Member

I think we can phase out the dict and gbtrees lookups as well, make a breaking note in the releases notes and call it a day 😄

I don't expect that many people put a dict or gbtree in the depcache and then do lookups on the keys of that stored value.

At least it should remove that funky digging into structures and assuming what module should be used for access to subkeys.

@mworrell mworrell merged commit f58e76c into zotonic:master Sep 19, 2022
@mworrell
Copy link
Member

Thank you @Maria-12648430 !

@mworrell
Copy link
Member

Removing the dict/gbtree sub-key lookups.

#17

@Maria-12648430
Copy link
Contributor Author

Removing the dict/gbtree sub-key lookups.

#17

ok, great, that simplifies a few things 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants