You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Hey @timges yeah you're right, it's a bit of a code smell to repeat that ternary operator everywhere in the codebase.
Even if we were to abstract that to a simple utility function, it still wouldn't really improve it all that much because the root of the code smell is that fact that we have to pass an option "usesDtcg" all throughout the codebase constantly because so much of the codebase needs to access the token type / value.
I think, in hindsight, that it would have been better to just introduce a built-in processing step that always converts any dictionary to DTCG syntax no matter what. That way, we always only check for $value/$type/$description.
We may even be able to do this in a non-breaking change in v4. I'm gonna create an issue for this at least because it would certainly clean up the code by a lot and prevent us needing to do a bunch of DTCG syntax integration tests, we only do that in a limited amount of tests, so DTCG syntax test coverage just really isn't great right now.
Edit: Actually, this would always be a breaking change because in custom hooks that folks register, they'd get the tokens in DTCG format always, so it breaks the hooks for folks that still only use the old syntax. So this'll be a v5 improvement then..
Hey @timges yeah you're right, it's a bit of a code smell to repeat that ternary operator everywhere in the codebase.
Even if we were to abstract that to a simple utility function, it still wouldn't really improve it all that much because the root of the code smell is that fact that we have to pass an option "usesDtcg" all throughout the codebase constantly because so much of the codebase needs to access the token type / value.
I think, in hindsight, that it would have been better to just introduce a built-in processing step that always converts any dictionary to DTCG syntax no matter what. That way, we always only check for $value/$type/$description.
We may even be able to do this in a non-breaking change in v4. I'm gonna create an issue for this at least because it would certainly clean up the code by a lot and prevent us needing to do a bunch of DTCG syntax integration tests, we only do that in a limited amount of tests, so DTCG syntax test coverage just really isn't great right now.
Edit: Actually, this would always be a breaking change because in custom hooks that folks register, they'd get the tokens in DTCG format always, so it breaks the hooks for folks that still only use the old syntax. So this'll be a v5 improvement then..
Originally posted by @jorenbroekema in #1305 (comment)
The text was updated successfully, but these errors were encountered: