Update JS backend code to ES6. #20
Conversation
Modernizes and makes the code more readable. No intentional changes to functionality, other than bugfixes and updates to comments.
|
Actually, now that I think of it, it does change the functionality--I changed |
| @@ -148,7 +148,7 @@ function read_files(repo_loc, callback) { | |||
| } | |||
|
|
|||
| for (let timing of times) { | |||
| for (let phase in timing.times) | |||
| for (let phase in timing.times) { | |||
There was a problem hiding this comment.
I'm a bit confused about what is going on here, you seem to add an unbalanced brace - is this just in the wrong commit?
There was a problem hiding this comment.
Yeah, I'm not sure how this snuck in the previous commit.
|
I'm a bit confused by commit 3 - I don't see how that fixes the bug, perhaps the fix is in a different commit or something? |
|
Could you stick to spaces rather than hard tabs please? As well as trying to keep consistent, it is screwing up the indentation in GitHub :-s |
| summary['rustc'] = []; | ||
| summary['benchmarks'] = []; | ||
| summary.rustc = []; | ||
| summary.benchmarks = []; |
There was a problem hiding this comment.
This could be an object literal, rather than setting the fields outside the object
|
Commit 3 fixes it by introducing these lines which Sure, I can change that. I'm guessing Sublime was feeling overzealous about spacing. |
|
Looks good, but is pretty hard to review because of the spaces -> tabs change. Is there a reason to change read_files to blocking? I don't think it matters which way it is written, but the old way seems like more idiomatic node.js. |
|
Ah, and I guess you had previously unrolled the loop into b5ecf2b#diff-8a18e115a8fa64b81ca1f7a5167bf197R221 which finds the median by actually looking at three different entries rather than the same entry three times? |
|
Fixed the spaces -> tabs (hopefully). The read_files change was done to make sure that there is never a partial update state (whether on init or after a github webhook update). I'm not sure if it matters, but with the old code, if someone requested the page soon after the webhook came in it wouldn't display the right data (at least I think so, not actually certain about this--I'm not sure exactly how Node's event loop works). |
| @@ -1,2 +1,2 @@ | |||
| sudo npm install nodegit http url async path fs sortedlist | |||
| sudo npm install nodegit sortedlist | |||
There was a problem hiding this comment.
These are all core node modules, no need to install them.
|
Yeah, I'm not 100% happy with the current median computation code, but I don't think the loop was ever in that code (see https://github.com/rust-lang-nursery/rustc-perf/pull/20/files#diff-8a18e115a8fa64b81ca1f7a5167bf197L197), I just changed it to 4 lines instead of one for readability. |
| let date_components = [0, 0, 0, 0, 0, 0]; | ||
| let actual = header.date.split("-").map(comp => parseInt(comp, 10)); | ||
|
|
||
| for (let i = 0; i < actual.length; i++) { |
There was a problem hiding this comment.
Either works, but the above makes it clear that it is an array. I would've used for-of syntax, but we need the index to update date_components. Let me know if you prefer for-in.
I also think that at least to some extent, the JS community is moving away from for-in syntax in preference to for-of, c-style for, and for-of with Object.keys. There is also this to consider, although most of those concerns are probably not applicable in this specific case.
|
OK, I think this looks good, thanks. There's a few comments inline. I'm not totally confident that the changes won't change anything though, especially as we don't have tests (my fault, sorry). Did you manage to test this locally? |
|
From visual-based testing, everything seemed equivalent. If you'd like I can try to run wget and diff the results before and after? |
|
Pushed fixes to inline comments and linter warnings. |
|
I'm not sure why I failed to notice this before (possibly because I was testing with an outdated version of the input data), but the changes to indexing result in a massive difference currently: 2.8% on the summary page at perf.rust-lang.org and 11.4% in this branch, at least from what I can tell. |
|
Closing in favour of #22 as discussed on irc |
Modernizes and makes the code more readable. No intentional changes to
functionality (other than fixing the median functionality as discussed on IRC). Also updated the 60 day comment to 30 days.