Conversation
meta-cla
bot
added
the
CLA Signed
cbalioglu
left a comment
cbalioglu
left a comment
There was a problem hiding this comment.
Thanks for the PR, however I don't think we should be overriding docstrs of the base MetricRecorder methods in each subclass.
We also certainly should not be adding docstrs to private methods/functions. If they are not descriptive enough, we should ideally think about how we can refactor them or if that is not feasible just have some regular inline comments instead of docstrs.
My suggestion is that if you think there is more info needed to make the API reference more clear, to add them either to MetricRecorder or to the class docstr of corresponding metric recorders.
Having said that, we will likely have to have a lot more documentation on how these metric recorders behave. I am fine with adding more preliminary docs to them for now, but note that we will certainly significantly overhaul the current docs in the near future.
| def record_metric_values( | ||
| self, category: str, values: Mapping[str, object], step_nr: int | None = None | ||
| ) -> None: | ||
| """ |
There was a problem hiding this comment.
I don't think we need to add documentation for overridden methods. The docstr of the method on the corresponding interface (in this case MetricRecorder) should be descriptive enough to explain the expected behavior of the metod.
| @override | ||
| def close(self) -> None: | ||
| """ | ||
| Closes and removes the :class:MetricRecorder |
| self, category: str, values: Mapping[str, object], step_nr: int | None = None | ||
| ) -> None: | ||
| """ | ||
| Gets, sorts, and maps metrics, their values, and descriptions to a ``dict`` |
There was a problem hiding this comment.
Again, no need to override the docstr of base interface. Any behavior specific to this subclass should ideally be described in the class docstr.
| values_and_descriptors.sort(key=lambda p: (p[1].priority, p[1].display_name)) | ||
|
|
||
| def sanitize(value: object) -> object: | ||
| """ |
There was a problem hiding this comment.
This is an inline function. No need for docstr.
| raise_operational_system_error(ex) | ||
|
|
||
| def _get_stream(self, category: str) -> TextIO: | ||
| """ |
There was a problem hiding this comment.
No need to document private functions.
| @override | ||
| def close(self) -> None: | ||
| """ | ||
| Closes the stream and clears object |
Removed redundant docstrings
Removed redundant docstrings
2 participants
What does this PR do? Please describe:
Added docstrings to files in the metrics module
Fixes #1360
Check list: