-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Keep csv files open #1522
Keep csv files open #1522
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1522 +/- ##
==========================================
+ Coverage 81.58% 81.94% +0.35%
==========================================
Files 28 28
Lines 2466 2509 +43
Branches 382 381 -1
==========================================
+ Hits 2012 2056 +44
+ Misses 359 358 -1
Partials 95 95
Continue to review full report at Codecov.
|
Does this also split stats into stats and stats_csv? It might make sense, but as I dont really know the web interface code very well, I can't really judge if that is a good idea. |
Yes, it splits out the csv generation. I think it gives better readability, but I can merge it back into stats if you prefer. |
Yes, I would prefer keeping them in the same file (but we can change it later maybe) |
Let me rebase on master first.
…On Tue, 18 Aug 2020, 10:55 Lars Holmberg, ***@***.***> wrote:
Is this ok to merge now?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1522 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAMCZSN7JHC3Y5O5SZAMQPDSBI6ZPANCNFSM4QAX2LHA>
.
|
|
I think I will be to blame for the stats-csv stuff regardless how it is un-split, since all lines will be indented under the class. |
Created a new single commit and force pushed. Should keep the blame changes to a minimum, although not sure there is much difference. |
LGTM. Thanks! |
The intent of is to reduce os calls and disk IO.
This code goes on top of my other PR #1504, so submitting as draft.