Fix/telemetry #13
Fix/telemetry #13
Conversation
| RUBY_TO_API_KEY_MAPPING = T.let({ | ||
| 'enabled' => 'is_enabled' | ||
| }.freeze, | ||
| T::Hash[String, String]) |
There was a problem hiding this comment.
in order to be more idiomatic for ruby, while using the required Mastra API prop. I explored putting it directly in the camelized options with an if clause, but if in the future we adapt other API methods that are not idiomatic for Ruby, this way we just add it to the hash.
I'm changing this because before we had the is_enabled, which was the Mastra prop name, but you told me to use enabled to be more idiomatic ( which I think is a good tip tbh ).
Any other approach you think would be better?
There was a problem hiding this comment.
Yes, sorry. What I mean is that this feels very complex just for one particular case. When you pass the telemetry settings to the options, couldn't the settings itself know how to serialize themselves (so returning a hash with is_enabled) instead of having the client know about this?
There was a problem hiding this comment.
Makes hella more sense, yup, ty
🚪 Why?
This Pull Request addresses feedback from a previous PR by refining the implementation and aligning it with requested changes. The goal is to improve code clarity, idiomatic usage, and test coverage.
🔑 What?
This PR introduces the following changes based on reviewer comments:
is_enabledtoenabledfor a more idiomatic Ruby style.contextsto the telemetry settings specs to improve tests behavior.🏡 Context