-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Filter only multitable placeholder 172658719 #196
Conversation
): | ||
self._cube_response_arg = response | ||
self._transforms_dict = {} if transforms is None else transforms | ||
self._first_cube_of_tab = first_cube_of_tab | ||
self._cube_idx_arg = cube_idx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why self._cube_idx_arg
and not self._cube_idx
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cube_idx_arg
(the cube_idx
argument) can be None
or int
. Cube index is always an int. So the _arg
suffix cues the reader to look for some reason why the argument doesn't directly become the served value.
@@ -233,6 +231,11 @@ def counts(self): | |||
def counts_with_missings(self): | |||
return self._measure(self.is_weighted).raw_cube_array | |||
|
|||
@lazyproperty | |||
def cube_index(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
out of curiosity: does this property returns a cube idx or an offset?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idx
and "offset" are generally synonymous. In this case, it is always a positive int value, the first cube returning 0
, the next 1
, etc. It is never None
however. We use None
on the cube_idx_arg
to indicate that the cube appears by itself, that is, that the cube count is 1
. That informs the ._is_multi_cube
property and ca-as-0th detection, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -392,6 +400,10 @@ def but_it_raises_on_other_cube_response_types(self, cube_response, expected_val | |||
|
|||
# fixture components --------------------------------------------- | |||
|
|||
@pytest.fixture | |||
def _cube_dict_prop_(self, request): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this fixture component mandatory considering that it's used only in it_knows_its_title
test? IMHO I would mock that property in the test directly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not at the moment, but that's because the unit test coverage is still very spotty on this class. I made the judgement to include it since that property is used in like four other methods so will be needed when those unit tests are added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Convention dictates all test classes for a module reside in the same corresponding test module.
Avoid using unshared parameterized fixtures distant from the test that uses it when possible (when cases take less than a dozen lines or so). Use `@pytest.mark.parametrize` to provide case data instead in those cases.
These maybe used to work in the old slice implementation but `row_index` and custom-baseline have not been needed in the new implementation.
Tighten identification of numeric-mean case to avoid false positives as we introduce filter-only multitable-template placeholders.
Unit-test coverage is spotty. Fill this one in because we need to adjust the method slightly in a subsequent commit.
Exports need to know the cube-index to inform certain rules like identifying filter-only multitable tiles. Replace `first_cube_of_tab` argument and instance variable with `cube_idx`, which provides more information.
Also a couple incidental fixes for small gaps uncovered along the way.
5aa58b3
to
44d0cd8
Compare
Add
_Strand.cube_index
and_Strand.title
for to enable handling of filter-only multitable-template placeholders.