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

Exclude resolvers without config from the chain #475

Closed

Conversation

ThinkChaos
Copy link
Collaborator

The code in this branch makes it so only actually configured resolvers are created and added to the chain.
This should simplify reading trace logs, and add a small speedup to queries (didn't attempt to benchmark).

The PR is not ready: tests do not pass yet, and that's a fair bit of work.
Is it worth it I spend time fixing the tests, is this something you'd be interested in merging?

- Runtime error if last resolver is a chained one
- Compile error if non-chained resolver used not at the end
- ChainBuilder ignores nil resolvers
- Make resolver constructors return nil if config is empty
@0xERR0R
Copy link
Owner

0xERR0R commented Apr 7, 2022

IMHO, I don't see any advantages for this:

  • each resolver implementation has more code now (check if passed configuration must be processed or is the configuration for each resolver "does nothing"), but the result is the same (returning nil and exclude the resolver from chain vs. resolver delegates to next without any operation)
  • Each resolver has a constructor function "NewXXX", which implies that the function returns a new instance. Returning nil is IMHO against this principle
  • regarding performance improvements: I think, it is not really measurable, if resolver does "nothing" it simple delegates to the next one
  • Regarding debugging: It depends, what do you mean with debugging. If you analyze log files -> OK, there may be more log entries, because not active resolvers log something. If you mean code debugging with debugger -> it depends on your tool. For example in Goland (aka. IntelliJ) you can create conditional breakpoints.

Maybe this will be addressed with #476?

@ThinkChaos
Copy link
Collaborator Author

I understand your concerns. Closing this is fine with me.
(Debugging I did mean end user via logs)

I think the ChainBuilder would be useful for #476. I can open a PR with just that if you agree.

@ThinkChaos ThinkChaos closed this Apr 9, 2022
@ThinkChaos ThinkChaos deleted the feat/no-unused-resolvers branch January 24, 2023 00:35
@ThinkChaos ThinkChaos restored the feat/no-unused-resolvers branch January 24, 2023 00:35
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