temporal icon indicating copy to clipboard operation
temporal copied to clipboard

Typed dynamic config

Open dnr opened this issue 1 year ago • 3 comments

What changed?

Dynamic config improvements:

  • Typed settings to prevent misuse
  • Moved defaults to where settings are defined
  • Code generation to make it easy to add new types and filters
  • Shorter names for readability

Why?

Making it easier to use, preventing misuse, making it easy to extend, easier to parse and generate documentation from, base for future enhancements (e.g. validation at load time)

How did you test it?

modified existing tests

Potential risks

typo or missed something in all the merges and refactors

dnr avatar Apr 17 '24 04:04 dnr

This PR has a delta of nearly 8000 changed lines (I'm lazily summing additions and subtractions).

Can you break this PR into a stack for your reviewers sanity? A stack like:

  1. The changes to the core dynamicconfig types, collections, etc
  2. The changes to all users of dynamic config
  3. Other cleanup and fixes (like your changes to EnableBatcher* above)

So we talked about the idea of making PRs to PRs, but I realized it won't work since the PR is from a fork and a PR to that would be in my forked repo :disappointed: I can remake it but then we'd lose the review history so far (maybe not a big deal)

Would you be okay with splitting it up into commits within one PR?

dnr avatar Apr 26 '24 06:04 dnr

This PR has a delta of nearly 8000 changed lines (I'm lazily summing additions and subtractions). Can you break this PR into a stack for your reviewers sanity? A stack like:

  1. The changes to the core dynamicconfig types, collections, etc
  2. The changes to all users of dynamic config
  3. Other cleanup and fixes (like your changes to EnableBatcher* above)

So we talked about the idea of making PRs to PRs, but I realized it won't work since the PR is from a fork and a PR to that would be in my forked repo 😞 I can remake it but then we'd lose the review history so far (maybe not a big deal)

Would you be okay with splitting it up into commits within one PR?

That'll work. I appreciate it

tdeebswihart avatar Apr 26 '24 16:04 tdeebswihart

That'll work. I appreciate it

I force-pushed with a split into 5 commits. I haven't merged with recent changes on main (new settings, mostly), I'll do that all at once right before commit to avoid duplicating work.

dnr avatar Apr 26 '24 21:04 dnr