{"comment":{"url":"https://api.github.com/repos/ramonhpr/knot-cloud-source/comments/24221722","html_url":"https://github.com/ramonhpr/knot-cloud-source/commit/4d764a5019d1c831d606e69046e12f2561c52d53#commitcomment-24221722","id":24221722,"user":{"login":"figueredo","id":2797607,"avatar_url":"https://avatars3.githubusercontent.com/u/2797607?v=4","gravatar_id":"","url":"https://api.github.com/users/figueredo","html_url":"https://github.com/figueredo","followers_url":"https://api.github.com/users/figueredo/followers","following_url":"https://api.github.com/users/figueredo/following{/other_user}","gists_url":"https://api.github.com/users/figueredo/gists{/gist_id}","starred_url":"https://api.github.com/users/figueredo/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/figueredo/subscriptions","organizations_url":"https://api.github.com/users/figueredo/orgs","repos_url":"https://api.github.com/users/figueredo/repos","events_url":"https://api.github.com/users/figueredo/events{/privacy}","received_events_url":"https://api.github.com/users/figueredo/received_events","type":"User","site_admin":false},"position":74,"line":111,"path":"lib/knotGatewayManager.js","commit_id":"4d764a5019d1c831d606e69046e12f2561c52d53","created_at":"2017-09-11T14:31:48Z","updated_at":"2017-09-11T14:31:48Z","author_association":"NONE","body":"This isn't doing what the method should do. While `splice` changes `gatewayDeviceMap`, `concat` doesn't. From the MDN docs (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/concat?v=b):\r\n\r\n> This method does not change the existing arrays, but instead returns a new array.\r\n\r\nWhat is happening here is:\r\n\r\n1. Removes element at `mapIndex` from `gatewayDeviceMap` and returns an array with it: `[removedEl]`\r\n2. Concatenates `data.mappings` with the result of `splice` and attribute it to `mapping`: `[removedEl, ...data.mappings]`\r\n\r\nAt the end you have:\r\n\r\n- `mapping`: `removedEl + data.mappings`\r\n- `gatewayDeviceMap`: previous `gatewayDeviceMap - removedEl`\r\n\r\nwhen it should be:\r\n`gatewayDeviceMap`: previous `gatewayDeviceMap - removedEl + data.mappings`\r\n\r\nI think you don't need to care about `mapping`, the user needs only to know if the operation succeeded or not. The user knows which data it sent to the function, so there isn't the need to return any data. Notice that the previous implementation doesn't return any data."}}
{"actor":{"display_login":"figueredo"}}