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

Isolate state management #19

Merged
merged 5 commits into from
Dec 26, 2023
Merged

Isolate state management #19

merged 5 commits into from
Dec 26, 2023

Conversation

mjheilmann
Copy link
Collaborator

@mjheilmann mjheilmann commented Dec 24, 2023

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

@@ -0,0 +1,18 @@
defmodule GrpcReflection.Service do
Copy link
Collaborator Author

@mjheilmann mjheilmann Dec 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the point in the code where we pivot to the Agent implementation, so external callers are insulated against internal changes

@mjheilmann mjheilmann marked this pull request as ready for review December 24, 2023 17:41
@zhihuizhang17
Copy link
Contributor

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

@mjheilmann
Copy link
Collaborator Author

mjheilmann commented Dec 26, 2023

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.

@mjheilmann mjheilmann merged commit a86e804 into main Dec 26, 2023
1 check passed
@mjheilmann mjheilmann deleted the mjheilmann/refactor_state_mgmt branch December 26, 2023 15:41
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.

Reflection State is split up
2 participants