Improve code quality in History (complexity) #182
Conversation
| // Rename deperecated apps | ||
| if ( app === "ZeroBin" ) { | ||
| app = "Message"; | ||
| } |
There was a problem hiding this comment.
This if block still needs to exist in the refactored version.
There was a problem hiding this comment.
We can remove this in a few weeks, but we need it at the moment -- I just need to find time to write and test a migration server side.
|
I have not tested this locally yet so issues may yet arise, but other than the comments above I think this looks good. |
|
Thanks! I'll fix issues from your comments :) |
|
I have a feeling that the complexity of this code has not decreased significantly. I would sign up for CodeClimate and point to your branch to see how much the complexity was reduced. |
|
@irdan Just C -> B in that case. It's mainly a test to setup correctly my environment. |
|
Code quality heuristics don't like having big blocks of code inside loops, which we often unintentionally ignored. Regardless, this should be easier to test. Speaking of which, have you gotten to setting up Karma yet? How do the tests look on this? |
|
@smcgregor I haven't found any test case for this callback function. Indeed, all tests are successful according to Karma. EDIT: will write some tests tomorrow |
|
For reference, here is the current build running from my fork: https://travis-ci.org/smcgregor/privly-applications/builds/52995589 |
| expect(buttons[1].getAttribute("data-canonical-href")).toMatch(/show.html\?privlyOriginalURL\=testUrl/); | ||
|
|
||
| for(var i = 1; i <= 3; i++) | ||
| expect(cells[i].textContent).not.toBe(""); // TODO unit tests for parseDate |
There was a problem hiding this comment.
For better or for worse, I believe CodeClimate will complain about not bracketing this.
|
New unit test looks good. With one more for exclusively |
|
Two tiny comments left. Also, make sure you comment when you push new commits, otherwise people don't get emailed about it. |
|
|
||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
So many PRs to get through, didn't mean to repeat this.
|
No problem! :) |
|
I tested locally and all is good. I am just waiting for the tests to run on Continuous Integration, then I'll merge. Note: You are likely going to have issues with your fork since you are developing on master. I recommend working on a feature branch. |
#155