Bug #12884
closed
- Status changed from New to In Progress
- Status changed from In Progress to New
- Assigned To set to Peter Amstutz
- Target version changed from 2018-01-17 Sprint to 2018-01-31 Sprint
- Status changed from New to In Progress
- Target version changed from 2018-01-31 Sprint to 2018-02-14 Sprint
Thought I fixed this but repository 404 error isn't being reported to the user, needs work.
The updates LGTM. One question about code style:
- File src/app/core/panels/my-apps-panel/arvados-apps-panel.service.ts:
- Line 90: Could
load_dir()
function be declared only once outside the .map() of .map()s, for example right below the getRootFolders()
level? At first glance it seems that it uses very few outside vars that could be passed as parameters, and we avoid declaring lots of times the same function and improve a lot the readability.
Lucas Di Pentima wrote:
The updates LGTM. One question about code style:
- File src/app/core/panels/my-apps-panel/arvados-apps-panel.service.ts:
- Line 90: Could
load_dir()
function be declared only once outside the .map() of .map()s, for example right below the getRootFolders()
level? At first glance it seems that it uses very few outside vars that could be passed as parameters, and we avoid declaring lots of times the same function and improve a lot the readability.
It calls itself recursively as well as capturing some variables from the outer scope. You're right there's a readability tradeoff here.
Just in case you're waiting for further feedback from me, this LGTM.
- Status changed from In Progress to Resolved
Also available in: Atom
PDF