-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add support for package extensions in show
method
#94
Add support for package extensions in show
method
#94
Conversation
* Julia code in `ext`: $(l_ext) lines ($(p_ext)% of `test` + `src` + `ext`) | ||
* Julia code in `test`: $(l_test) lines ($(p_test)% of `test` + `src` + `ext`) | ||
* documentation in `docs`: $(l_docs) lines ($(p_docs)% of `docs` + `src` + `ext`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure adding ext
to the denominator was the best thing to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, I'm not sure either. I think it is probably OK though, treating it similarly to source code, and expecting a package with a lot of extension code to need more documentation than one without it.
show
method
I think the CI failure on Julia 1.6 - windows-latest is not related to this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, but I suggested a change to reduce duplication
p_ext = @sprintf("%.1f", 100 * l_ext / (l_test + l_src + l_ext)) | ||
p_test = @sprintf("%.1f", 100 * l_test / (l_test + l_src + l_ext)) | ||
p_docs = @sprintf("%.1f", 100 * l_docs / (l_docs + l_src + l_ext)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should have
p_ext = @sprintf("%.1f", 100 * l_ext / (l_test + l_src + l_ext)) | |
p_test = @sprintf("%.1f", 100 * l_test / (l_test + l_src + l_ext)) | |
p_docs = @sprintf("%.1f", 100 * l_docs / (l_docs + l_src + l_ext)) | |
l_total = l_test + l_src + l_ext | |
p_ext = @sprintf("%.1f", 100 * l_ext / l_total) | |
p_test = @sprintf("%.1f", 100 * l_test / l_total) | |
p_docs = @sprintf("%.1f", 100 * l_docs / l_total) |
so that we don't have to change the total in multiple places every time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it okay to have p_docs
is larger than 100? Note that not all denominators are equal. Should we define l_total
with
l_total = l_test + l_docs + l_src + l_ext
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes, sorry, I missed that one was different
This PR adds support for package extensions by counting lines in the
ext
directory.Before this PR
After this PR