Add showTotal attribute to GraphIndicator #38
Conversation
- Add showTotal attribute to GraphIndicator class with default value True - Include showTotal in response when processing indicators - Add comprehensive tests for showTotal functionality in both graph_spec.py and processor_spec.py - Maintain backward compatibility with default showTotal=True
ecarreras
force-pushed
the
80812/add-show-total
branch
from
47fd77c to
f8b0ba5
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new showTotal attribute to the GraphIndicator class, allowing users to control whether the total value is displayed in indicator graphs. The attribute defaults to True when not specified and can be explicitly disabled by setting it to "0".
Key Changes:
- Added
showTotalattribute toGraphIndicatorclass with a default value ofTrue - Modified the
processmethod to always includeshowTotalin the response - Added comprehensive test coverage for default behavior, explicit enabling/disabling, and correct output
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
ooui/graph/indicator.py |
Implemented _show_total attribute parsing, property accessor, and updated process method to include showTotal in output |
spec/graph/graph_spec.py |
Added tests verifying default behavior, explicit enabling/disabling, and that showTotal is always present in responses |
spec/graph/processor_spec.py |
Updated existing test to expect showTotal=True by default and added test for showTotal="0" behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| result = graph.process(50, 100) | ||
| expect(result).to(have_key('showTotal')) | ||
| expect(result['showTotal']).to(be_true) | ||
|
|
There was a problem hiding this comment.
Trailing whitespace detected. This line should not have trailing whitespace.
| res['progressbar'] = self.progressbar | ||
| if self.show_percent: | ||
| res['showPercent'] = self.show_percent | ||
| res['showTotal'] = self.show_total |
There was a problem hiding this comment.
Inconsistent API design: showTotal is always included in the response (line 80), while showPercent (line 78-79) and progressbar (line 76-77) are only included when True. Consider applying the same pattern for consistency - either always include all three boolean flags, or only include them when True. This inconsistency could confuse API consumers.
| res['showTotal'] = self.show_total | |
| if self.show_total: | |
| res['showTotal'] = self.show_total |
This pull request introduces support for a new
showTotalattribute in the indicator graph type, allowing users to control whether the total value is displayed. By default,showTotalis enabled unless explicitly set to"0". The changes also include updates to processing logic and comprehensive tests to verify the new behavior.Indicator graph enhancements:
showTotalattribute to theIndicatorclass inooui/graph/indicator.py, defaulting toTrueunless specified otherwise in the XML. This allows users to toggle the display of the total value.show_totalproperty and updated theprocessmethod to includeshowTotalin the output only when enabled. [1] [2]Testing and validation:
spec/graph/graph_spec.pyto verify default behavior, explicit enabling/disabling, and correct output ofshowTotal.showTotalin processed graph results.spec/graph/processor_spec.pyto verify that disablingshowTotalvia XML omits it from the result. [1] [2]