elixir-grpc / grpc-reflection

elixir graph reflection support
Apache License 2.0
9 stars 6 forks source link

Isolate state management #19

Closed mjheilmann closed 8 months ago

mjheilmann commented 8 months ago

The PR cleans up the existing functionality through the following:

  1. Add a service-level interface so servers aren't calling Builder or Agent directly
  2. Create a State module, and move state mutation and accessor logic into it
  3. Remove Loader, as it is replaced by State
  4. Update Builder to mutate reflection state using State instead of directly
  5. Move small util functions out of Builder into Util
  6. Relocate Builder.Util, it is trivially a submodule and not a nested module
  7. Added references into the State struct, it could be useful for debugging
  8. Streamline Builder now that State holds references, merges state, etc
  9. Remove unhandled rescue clauses so that actual errors are exposed while building a reflection state

fixes #17

zhihuizhang17 commented 8 months ago

What about moving bulider.ex to the subfolder bulider too? As they share the same name.

mjheilmann commented 8 months ago

What about moving bulider.ex to the subfolder bulider too? As they share the same name.

Two reasons:

  1. style guide suggests aligning paths and module names, so path/to/module.ex contains defmodule Path.To.Module. I interpret this as meaning that if Builder was also in a folder named builder, then the module would need to be named Builder.Builder, and I don't want to do that.

  2. When trying to write clean and testable modules, establishing a clean and minimal interface is important, where the exposed functionality is documented, and implementation details are hidden. Builder isn't very complicated, but when following that pattern the interface should be visible and accessible rather than inside a folder with all the implementation details.