always make GuildRegistry hold Guild structs strictly

[ndac-todoroki]

Why

Some functions, for example Nostrum.Cache.Guild.GuildServer.get/1 is documented to return a %Guild{} struct, but sometimes it did not. This was because get/1 relies on transform/1, which relies on getting values from the Registry.
The Registry initially holds a Nostrum.Struct.Guild struct, by passing that struct to GuildRegister.create_guild_process/2, but after that almost all handle_calls updated the state with a simple map.
This becomes a problem such as: Dot-notations will raise an error when it was supposed to return a nil (if it was a struct). Since GUILD_UPDATE renewals the whole state, the entire thing becomes a map, and it breaks eg. Nostrum.Cache.Guild.GuildServer.get/1's type document.

What

• handle_calls' states are restricted to %Nostrum.Guild{}s
• always do to_struct/1 on updating and creating new elements
  • vals which supposed to be lists but having empty maps as default values inside to_struct/1, now have lists as default value

concerns

• should the default empty maps in to_struct/1 should be an empty struct?

[Kraigie]

Thanks!

[Kraigie]

Another issue I can see here (and it's replicated throughout the module) is that the info we're sending will not be in a struct. Here, the state of the guild will have this member as a struct, but the event that the user handles will be a map (when according to documentation it should be a struct).

Do you think it would be better to move the conversion logic to the client side of the server? We could also move it back to the dispatch module, although it may be weird there as the other caches are stored-ish in ETS and as such won't need this type of conversion. Of course we could also just do the conversion first on the server side.

Let me know your thoughts on this!

[Kraigie]

Definitely a derp moment from me here. Makes much more sense to store these as the proper type.

[Kraigie]

Good point on whether we should store these as structs. I think the answer is that they should be.

I don't think it's worth your time to go and change every instance of this. Soon I'd like to replace these methods with the macro found here.

[ndac-todoroki]

I realized that this might break things in many places, if we extend using structs instead of maps in many other places. Examples:

• code like this will end up replacing some vals with unexpected nils, because discord only sends us partial data
  • this code currently works because new_partial_member is not a struct but a map (though currently this Map.merge breaks the struct because new_partial_member has an unknown guild_id key, it's a different problem)
  • but if we convert those to structs more earlier, meaning new_partial_member will be a struct and thus Map.merge will be bad.
• to_struct functions which have other to_structs needs rewriting too because Map.update/3 won't work
  • although I think just making four patterns will do:
  1. when &1 is nil,
  2. &1 is already the struct type we want,
  3. &1 is a map,
  4. anything else (return error or set default empty struct)

I'm afraid any code currently relying to maps (calling Enumerable or Access, or expecting a KeyError) will no longer work. Is it good to keep doing this? :( @Kraigie

[jchristgit]

Thank you for the PR. We have updated the guild caching logic to use ETS, and the ETS table contains Nostrum.Struct.Guild structs. If there's still some missing functionality for you, feel free to open an issue!