feat: Add assertions to support MetricWriter
#129
Conversation
msto
force-pushed
the
ms_metric-writer-assertions
branch
from
35593a1
to
30447a9
Compare
| if dataclasses.is_dataclass(metric_class): | ||
| return [f.name for f in dataclasses.fields(metric_class)] | ||
| elif attr.has(metric_class): | ||
| return [f.name for f in attr.fields(metric_class)] |
There was a problem hiding this comment.
Why not use metric.header() method?
There was a problem hiding this comment.
Short answer: this is an artifact from the initial proof-of-concept for this class being implemented in https://github.com/msto/dataclass_io/ (i.e. only for dataclasses, not considering Metric)
Long answer: metric.header() and inspect.get_fields() have a lot of type: ignores , and I'd rather use an implementation that's type-safe. (I have a mental TODO to clean up the volume of type: ignores in the repo, and I can leave a TODO that this function should be deprecated/merged into inspect once that happens.)
There was a problem hiding this comment.
I think we should use it and fix it?
| assert False, "Unreachable" | ||
|
|
||
|
|
||
| def _assert_file_header_matches_metric( |
There was a problem hiding this comment.
just saying out loud this will not work if the path is a stream, like standard input, since it consumes the header.
There was a problem hiding this comment.
I can add an assertion that the path is not /dev/stdin. Are there other streams that are likely to be represented as a Path?
msto
force-pushed
the
ms_metric-writer-feature-branch
branch
from
032388d
to
78f2246
Compare
msto
force-pushed
the
ms_metric-writer-assertions
branch
from
30447a9
to
0fd747b
Compare
| TypeError: If the given class is not a Metric. | ||
| """ | ||
| if not _is_metric_class(cls): | ||
| raise TypeError(f"Not a dataclass or attr decorated Metric: {cls}") |
There was a problem hiding this comment.
How do we feel about _assert functions raising errors other than AssertionError? I would probably be surprised by this.
There was a problem hiding this comment.
I don't think assert should be used outside of unit tests, and I prefer to raise more specific exceptions.
The Google style guide recommends/requires this as well:
Do not use
assertstatements in place of conditionals or validating preconditions. They must not be critical to the application logic. A litmus test would be that theassertcould be removed without breaking the code.assertconditionals are not guaranteed to be evaluated. For pytest based tests,assertis okay and expected to verify expectations.
The primary reason to avoid the use of assert statements is that they are intended for debugging and can be disabled.
Additionally, using assert makes it easy to fail to cover all branches of a program:
# Covered by any unit test that calls the parent function
assert is_ok(foo), "Foo was not ok"
if not is_ok(foo):
# Not covered unless the condition where `is_ok(foo)` is False is tested explicitly
raise ValueError("Foo was not ok") I'm happy to rename the functions to something other than assert_* (I've used validate_* in the past), I was just trying to remain consistent with naming convention in the project.
There was a problem hiding this comment.
This might be a good case for having a custom exception:
class ValueAssertion(AssertionError, ValueError):
passThen you would catch the exception with either AssertionError or ValueError.
I do like having the explicit branch with if since code coverage analysis will identify it as a covered/uncovered branch depending on actual test coverage. With single-line statements (e.g. assert), you don't get any branch information so it is easy to forget to test the exception paths.
doc: update doc refactor: try-except read header doc: rm raises block from get_fieldnames refactor: set refactor: avoid dup assert
tests: cover
msto
force-pushed
the
ms_metric-writer-assertions
branch
from
0fd747b
to
beee947
Compare
| assert False, "Unreachable" | ||
|
|
||
|
|
||
| def _assert_file_header_matches_metric( |
I split the remaining code in #107 into two PRs to facilitate review.
This PR introduces several of the assertion methods used when constructing the
MetricWriter. (See #107 for how they are used in practice).