-
Notifications
You must be signed in to change notification settings - Fork 21
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
Issue 211: Style guide first draft #264
Conversation
Codecov Report
@@ Coverage Diff @@
## main #264 +/- ##
=======================================
Coverage 96.81% 96.81%
=======================================
Files 15 15
Lines 1570 1570
=======================================
Hits 1520 1520
Misses 50 50 |
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 8a767e4 is merged into main: |
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.
Seems like a start, but needs some work?
I also think there's generally room to be aspirational here - have some rules we want to enforce, but that the code doesn't yet comply with. Helps us fix things as refactors go along, not introduce more work when we finally get around to it, etc.
Co-authored-by: Carl A. B. Pearson <pearsonca@users.noreply.github.com>
Co-authored-by: Carl A. B. Pearson <pearsonca@users.noreply.github.com>
Co-authored-by: Carl A. B. Pearson <pearsonca@users.noreply.github.com>
Thanks Carl, some nice comments. I agree there is more we could cover but I think that needs to be discussed by a wider group first (so in a new issue, which you might like to write?) and potentially on a call if people feel strongly (sounds like a fun time to me). I'll make changes and then ping for another review. |
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 8a767e4 is merged into main: |
I think I have addressed the majority of both of your points. I agree there is more than can be done here on an aspirational level but quite a lot of that involves choices which really need to be made by the wider community (i.e. so we need issues to discuss and maybe bring up at a community meeting). |
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 8a767e4 is merged into main: |
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.
Minor changes proposed; otherwise, seems like a good start.
Co-authored-by: Carl A. B. Pearson <pearsonca@users.noreply.github.com>
Co-authored-by: Carl A. B. Pearson <pearsonca@users.noreply.github.com>
Co-authored-by: Carl A. B. Pearson <pearsonca@users.noreply.github.com>
Co-authored-by: Carl A. B. Pearson <pearsonca@users.noreply.github.com>
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 8a767e4 is merged into main: |
Description
This PR closes #211.
I have introduced a first pass at a
STYLE_GUIDE.md
which details out input and output requirements as well as some internal code expectations. It clearly states we depend on thetidyverse
style guide. This new style guide is now linked from theCONTRIBUTING.md
.This PR also partially addresses #234 but more discussion and work may be needed (not for this PR).
Checklist
NEWS.md
and theDESCRIPTION
.