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

Ml add more aggregate stats #5

Merged
merged 3 commits into from
Oct 14, 2020

Conversation

magneland
Copy link
Contributor

This adds sub-totals for code and tests stats to the output.

@bleonard
Copy link
Contributor

bleonard commented Oct 7, 2020

I missed this (for 3 years!)
Want to still merge this in?

@etagwerker seems to be using this. Want to test it out?

Copy link
Member

@etagwerker etagwerker left a comment

Choose a reason for hiding this comment

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

@magneland Would you be open to adding more tests for this new behavior?

I'm having a hard time understanding what it does (I'm new to this codebase and I'd like to help)

@magneland
Copy link
Contributor Author

@magneland Would you be open to adding more tests for this new behavior?

I'm having a hard time understanding what it does (I'm new to this codebase and I'd like to help)

For reference here is the original railties implementation:
https://github.com/rails/rails/blob/55badbba3be76172a10519488c2a2747a0552728/railties/lib/rails/code_statistics.rb

So this codebase actually has zero tests, and I think that is actually OK because it's a command line tool that prints a bit of information about a Rails app.

If this is still useful I think it should be sufficient to just run this branch and master branch on various repos and diff the output.

end
print_line("Code", @code_total)
print_line("Tests", @tests_total)
print_line("Total", @grand_total)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the main change.
The output changes from:

| Total                | 64560 | 51668 |     685 |    4354 |   6 |     9 |

to:

| Code                 |   17640 |   13748 |     338 |    1557 |   4 |     6 |
| Tests                |   46920 |   37920 |     347 |    2797 |   8 |    11 |
| Total                |   64560 |   51668 |     685 |    4354 |   6 |     9 |

@etagwerker
Copy link
Member

@magneland Cool, I just tested this in a sample application and it looks good. 👍

@bleonard I think this would be a good addition to the project.

@etagwerker etagwerker merged commit 3e1249c into fastruby:master Oct 14, 2020
@etagwerker
Copy link
Member

@magneland Thanks for your contribution! @bleonard just gave me access to merge this. 😄

@magneland magneland deleted the mlAddMoreAggregateStats branch October 14, 2020 19:54
etagwerker added a commit that referenced this pull request Oct 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants