Fix typing visibility for ExporterConfig #232
Merged
+168
−168
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dyastremsky
commented
Dec 25, 2024
| config.stats = stats.stats_dict | ||
| config.metrics = stats.metrics | ||
| config.args = args | ||
| assert isinstance(stats.metrics, Metrics) |
There was a problem hiding this comment.
These asserts are necessary because the current architecture means that stats.metrics can be Metrics or TelemetryMetrics. It is beyond the scope of this PR to separate the two, but we do need asserts like this to check the correct type is used and to not result in type checking failures. It was previously opaque that we were using two types in the exporter config.
We could always tackle it in a future cleanup effort if it causes issues, but making the typing transparent should help.
matthewkotila
approved these changes
Dec 30, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ExporterConfig was previously functioning as a dataclass without being one. In its init, all the types were set to None, even though the functions calling the config expected them to actually be set to a type. In other words, the type hint listed None but it was never actually supposed to be None. This resulted in Pylance errors everywhere we used this class's data objects and unclear visibility into the types of each object. This slowed development and was likely to lead to failures, albeit it made testing a bit easier (only the necessary data had to be set).
This pull request changes ExporterConfig to be a datatype and cleans up the type hints. It also adds a test util function to create a sample exporter config to keep the tests concise, create a single source of truth for changing the ExporterConfig in unit testing. That should make the unit testing more flexible and faster to update if this class changes in the future.