Skip to content
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

Allow passing multiple file names to graph, to get multiple lines in HTML #206

Merged
merged 9 commits into from
Aug 25, 2023

Conversation

devdanzin
Copy link
Collaborator

This PR addresses a TODO comment in graph.py: "Add multiple lines for multiple files". It turns path from a single argument into a variadic argument and plots lines for each file passed (and for those files in any directories passed too). If --aggregate is used, it aggregates the data for any directories passed, but also displays individual lines for any files passed (we can't easily aggregate those, as the aggregated data is recorded on build).

 wily graph .\src\wily\cache.py .\src\wily\state.py .\src\wily\commands\report.py .\src\wily\commands\diff.py .\src\wily\commands\index.py -m loc --all

multi_graph_files

 wily graph .\src\wily\cache.py .\src\wily\state.py .\src\wily\commands\report.py .\src\wily\commands\diff.py .\src\wily\commands\index.py .\src\wily\operators\ -m loc --all

multi_graph_files_and_directory

wily graph .\src\wily\cache.py .\src\wily\state.py .\src\wily\commands\report.py .\src\wily\commands\diff.py .\src\wily\commands\index.py .\src --aggregate -m loc --all

multi_graph_files_and_aggregated_directory

To achieve this, an incompatible change is needed: metrics, which previously was a variadic argument, now is a required option instead (which means it's always necessary to pass -m metric_name). To pass two metrics, users need to provide them as comma-separated values (same as in diff). Another way to go about this would be to add an explicit z_axis option, so when users want a metric in Z axis they provide it as this option.

Listing multiple file names in the graphs title simply doesn't work after a handful of files. So this PR avoids printing file names altogether when more than one path is supplied.

I like the comma-separated option because it matches diff and also because I'd like to introduce a new command using the same solution (it takes arbitrary number of paths and metrics, so it'll need a way to pass multiple metrics as non-variadic argument or option).

@codecov-commenter
Copy link

codecov-commenter commented Aug 24, 2023

Codecov Report

Merging #206 (3a62254) into master (acf39d2) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #206      +/-   ##
==========================================
+ Coverage   95.46%   95.48%   +0.01%     
==========================================
  Files          26       26              
  Lines        1346     1351       +5     
  Branches      287      287              
==========================================
+ Hits         1285     1290       +5     
  Misses         37       37              
  Partials       24       24              
Files Changed Coverage Δ
src/wily/__main__.py 95.69% <100.00%> (ø)
src/wily/commands/graph.py 100.00% <100.00%> (ø)
Copy link
Owner

@tonybaloney tonybaloney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Great improvement

@tonybaloney tonybaloney merged commit 07fc855 into tonybaloney:master Aug 25, 2023
20 checks passed
@devdanzin devdanzin deleted the multi_graph branch August 26, 2023 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants