feat: weaponize ASCII sparklines#18
Conversation
|
🤖 Hi @palladius, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR successfully extracts the ASCII sparkline generation logic into a generic, reusable script in the monitoring-graphs skill and adds visual framing with pipe wrappers. The changes improve code modularity and enhance the visual feedback for SRE investigations.
🔍 General Feedback
- Modularity: Excellent job moving the generic sparkline logic to a standalone script. This makes it much easier to use sparklines in other parts of the extension.
- Aesthetics: The addition of the
|wrappers and the switch to space for the baseline character makes the sparklines much easier to read in terminal outputs. - Consistency: The updates to
export_timeseries_to_csv.pycorrectly leverage the new script's logic while maintaining backward compatibility in terms of output format.
There was a problem hiding this comment.
🟢 The extraction of sparkline generation into a generic script is a great improvement for reusability across the extension.
However, consider adding a basic unit test for this new script (e.g., in a test/ directory) to ensure the ASCII generation remains consistent as you evolve it.
| normalized = np.round((binned - vmin) / (vmax - vmin) * 7).astype(int) | ||
| chars = ['_', '▂', '▃', '▄', '▅', '▆', '▇', '█'] | ||
| return "".join([chars[i] for i in normalized]) | ||
| chars = [' ', '▂', '▃', '▄', '▅', '▆', '▇', '█'] |
There was a problem hiding this comment.
🟢 This change from _ to (space) for the lowest bucket improves the visual clarity of the sparkline when wrapped in | pipes, as requested.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
LGTM |
1 participant
Extract ASCII sparklines generation to a generic script in monitoring-graphs and add pipes as wrappers. Closes #17