Added label feature for the techui.yaml#216
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #216 +/- ##
==========================================
+ Coverage 94.66% 94.76% +0.09%
==========================================
Files 10 10
Lines 825 859 +34
==========================================
+ Hits 781 814 +33
- Misses 44 45 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| suffix = "" | ||
| suffix_label = "" | ||
|
|
||
| # Try to get name from child labels if they exist, |
There was a problem hiding this comment.
Small gripe, but the indentation here is wrong 😅
| if ( | ||
| name.removeprefix(":").removesuffix(":") | ||
| in component.child_labels.keys() | ||
| ): | ||
| name = component.child_labels[name.removeprefix(":").removesuffix(":")] |
There was a problem hiding this comment.
As this is reused, it would be tidier to put it in a temporary variable, e.g.
| if ( | |
| name.removeprefix(":").removesuffix(":") | |
| in component.child_labels.keys() | |
| ): | |
| name = component.child_labels[name.removeprefix(":").removesuffix(":")] | |
| clean_name = name.removeprefix(":").removesuffix(":") | |
| if raw_name in component.child_labels.keys(): | |
| name = component.child_labels[clean_name] |
| height, width = self._get_group_dimensions(self.widgets) | ||
|
|
||
| if ( | ||
| screen_name in self.components |
There was a problem hiding this comment.
| screen_name in self.components | |
| screen_name in self.components.keys() |
|
|
||
| @pytest.fixture | ||
| def generator(): | ||
| def generator(builder: Builder): |
There was a problem hiding this comment.
I would prefer we didn't pass the Builder fixture into the Generator fixture. Can you make a mock components dict instead? Or even make a fixture that returns a mock components list as I think this is used elsewhere too
There was a problem hiding this comment.
Since I am trying to use the test-services techui-yaml, is it fine if I have the components fixture take in builder instead? Since that's how it's also done in the actual codebase?
|
|
||
| test_json_map = builder_with_test_files._generate_json_map( | ||
| screen_path.absolute(), dest_path | ||
| screen_path.absolute(), dest_path, builder_with_test_files.conf.components |
There was a problem hiding this comment.
| def test_get_labels(builder_with_test_files): | ||
| display_name = _get_labels( | ||
| "motor", | ||
| builder_with_test_files.conf.components, | ||
| None, | ||
| None, | ||
| ) | ||
| assert display_name == "Motor Stage" | ||
|
|
||
|
|
||
| def test_get_labels_child_labels(builder_with_test_files): | ||
| display_name = _get_labels( | ||
| "X", | ||
| builder_with_test_files.conf.components, | ||
| current_component_name="motor", | ||
| display_name="X", | ||
| ) | ||
| assert display_name == "X1" | ||
|
|
||
|
|
||
| def test_get_labels_child_labels_with_name_already_pregenerated( | ||
| builder_with_test_files, | ||
| ): | ||
| display_name = _get_labels( | ||
| "X1", | ||
| builder_with_test_files.conf.components, | ||
| current_component_name="motor", | ||
| display_name="X", | ||
| ) | ||
| assert display_name == "X1" |
There was a problem hiding this comment.
These tests would need to be changed re- https://github.com/DiamondLightSource/techui-builder/pull/216/changes#r3193774851
| generator._get_group_dimensions = Mock(return_value=(600, 400)) | ||
| generator.build_groups(screen_name) |
There was a problem hiding this comment.
Minor tidiness adjustment 😅
| generator._get_group_dimensions = Mock(return_value=(600, 400)) | |
| generator.build_groups(screen_name) | |
| generator._get_group_dimensions = Mock(return_value=(600, 400)) | |
| generator.build_groups(screen_name) |
| generator._get_group_dimensions = Mock(return_value=(600, 400)) | ||
| generator.build_groups(screen_name) |
There was a problem hiding this comment.
| generator._get_group_dimensions = Mock(return_value=(600, 400)) | |
| generator.build_groups(screen_name) | |
| generator._get_group_dimensions = Mock(return_value=(600, 400)) | |
| generator.build_groups(screen_name) |
| or existing display_name if name_elem is None. | ||
| """ | ||
| if name_elem is not None: | ||
| if name_elem in component.keys() and component[name_elem].label is not None: |
There was a problem hiding this comment.
Can you write a test for if name_elem exists but it isn't in component.keys()?
| if name_elem is not None: | ||
| if name_elem in component.keys() and component[name_elem].label is not None: | ||
| display_name = component[name_elem].label | ||
| elif current_component_name is not None and ( |
There was a problem hiding this comment.
I think the indentation here is wrong maybe? Also can you write a test for if current_component_name isn't None but it isn't in the component dict?
reverting an unnecessary change modified generate_tests for coverage generate_test coverage Removed unneeded prints and logs
| continue | ||
| self.widgets.append(new_widget) | ||
|
|
||
| def build_groups(self, screen_name: str): |
There was a problem hiding this comment.
Instead of creating a new class var for self.components, why not do a similar thing to build_wdiegst() where screen_components is passed in from builder?
You would need to rework the logic below to work for a list, or would have to refactor the code in create_screens() in builder.py to be a dict
2 participants
The changes include adding fields labels and child labels to techui.yaml components, which get passed as into the component models and entities. Then from entities, we get generated screens that take into consideration the child labels for the names of the screens, and also the main label instead of a key of components in the yaml file. After the screens are generated, the generate_json_map uses a new function _get_labels to fetch the labels from the techui.yaml, and places them where needed, taking precedence before what is already in the screens.