{"action":"created","issue":{"url":"https://api.github.com/repos/phetsims/wave-interference/issues/278","repository_url":"https://api.github.com/repos/phetsims/wave-interference","labels_url":"https://api.github.com/repos/phetsims/wave-interference/issues/278/labels{/name}","comments_url":"https://api.github.com/repos/phetsims/wave-interference/issues/278/comments","events_url":"https://api.github.com/repos/phetsims/wave-interference/issues/278/events","html_url":"https://github.com/phetsims/wave-interference/issues/278","id":392699388,"node_id":"MDU6SXNzdWUzOTI2OTkzODg=","number":278,"title":"3rd-party code in DiffractionScreenView","user":{"login":"pixelzoom","id":3046552,"node_id":"MDQ6VXNlcjMwNDY1NTI=","avatar_url":"https://avatars1.githubusercontent.com/u/3046552?v=4","gravatar_id":"","url":"https://api.github.com/users/pixelzoom","html_url":"https://github.com/pixelzoom","followers_url":"https://api.github.com/users/pixelzoom/followers","following_url":"https://api.github.com/users/pixelzoom/following{/other_user}","gists_url":"https://api.github.com/users/pixelzoom/gists{/gist_id}","starred_url":"https://api.github.com/users/pixelzoom/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/pixelzoom/subscriptions","organizations_url":"https://api.github.com/users/pixelzoom/orgs","repos_url":"https://api.github.com/users/pixelzoom/repos","events_url":"https://api.github.com/users/pixelzoom/events{/privacy}","received_events_url":"https://api.github.com/users/pixelzoom/received_events","type":"User","site_admin":false},"labels":[{"id":711960331,"node_id":"MDU6TGFiZWw3MTE5NjAzMzE=","url":"https://api.github.com/repos/phetsims/wave-interference/labels/dev:code-review","name":"dev:code-review","color":"104E8B","default":false}],"state":"open","locked":false,"assignee":{"login":"samreid","id":679486,"node_id":"MDQ6VXNlcjY3OTQ4Ng==","avatar_url":"https://avatars0.githubusercontent.com/u/679486?v=4","gravatar_id":"","url":"https://api.github.com/users/samreid","html_url":"https://github.com/samreid","followers_url":"https://api.github.com/users/samreid/followers","following_url":"https://api.github.com/users/samreid/following{/other_user}","gists_url":"https://api.github.com/users/samreid/gists{/gist_id}","starred_url":"https://api.github.com/users/samreid/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/samreid/subscriptions","organizations_url":"https://api.github.com/users/samreid/orgs","repos_url":"https://api.github.com/users/samreid/repos","events_url":"https://api.github.com/users/samreid/events{/privacy}","received_events_url":"https://api.github.com/users/samreid/received_events","type":"User","site_admin":false},"assignees":[{"login":"samreid","id":679486,"node_id":"MDQ6VXNlcjY3OTQ4Ng==","avatar_url":"https://avatars0.githubusercontent.com/u/679486?v=4","gravatar_id":"","url":"https://api.github.com/users/samreid","html_url":"https://github.com/samreid","followers_url":"https://api.github.com/users/samreid/followers","following_url":"https://api.github.com/users/samreid/following{/other_user}","gists_url":"https://api.github.com/users/samreid/gists{/gist_id}","starred_url":"https://api.github.com/users/samreid/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/samreid/subscriptions","organizations_url":"https://api.github.com/users/samreid/orgs","repos_url":"https://api.github.com/users/samreid/repos","events_url":"https://api.github.com/users/samreid/events{/privacy}","received_events_url":"https://api.github.com/users/samreid/received_events","type":"User","site_admin":false}],"milestone":null,"comments":0,"created_at":"2018-12-19T17:14:21Z","updated_at":"2018-12-21T17:59:38Z","closed_at":null,"author_association":"CONTRIBUTOR","body":"Related to #259 code review.\r\n\r\nReview of the 'Diffraction' screen was not part of this review, since it's not included in 1.0.  But this bit of odd code in `DiffractionScreenView` was noticed, and generated a discussion that's worth capturing here.\r\n\r\nThis was the code in questions. `$h` seemed like a very odd function name.\r\n\r\n```js\r\n      const $h = function( k, l ) {\r\n        if ( arguments.length === 0 ) {\r\n          return h_hats;\r\n        }\r\n\r\n        const idx = k * dims[ 0 ] + l;\r\n        return h_hats[ idx ];\r\n      };\r\n```\r\n\r\nSlack discussion identified this as code that was copied from GitHub project https://github.com/turbomaze/JS-Fourier-Image-Analysis.  Here's the Slack discussion:\r\n\r\n<details>\r\n<summary>Slack discussion</summary>\r\n\r\nChris Malley [4:19 PM]\r\nI see this in DiffusionScreenView: `const $h = function( k, l ) {` is that legal?\r\n\r\n\r\nSam Reid [4:20 PM]\r\nchecking….\r\n\r\n\r\nChris Malley [4:20 PM]\r\nAnd I know I’m not supposed to review diffusion.\r\n\r\n\r\nSam Reid [4:21 PM]\r\nAre you asking if JS variable names can start with the `$` character?\r\n\r\n\r\nChris Malley [4:21 PM]\r\nYes, I’ve never seen that. And why?\r\n\r\n\r\nSam Reid [4:22 PM]\r\nvariables that begin with `$` are common when using jquery, see https://stackoverflow.com/questions/205853/why-would-a-javascript-variable-start-with-a-dollar-sign\r\n\r\nThis code was adapted from an example that uses an open-source fast fourier transform library.\r\n\r\n\r\nChris Malley [4:23 PM]\r\nAnd your reason for using `$h` here?\r\n\r\n\r\nSam Reid [4:23 PM]\r\n> This code was adapted from an example that uses an open-source fast fourier transform library.\r\n\r\n\r\nChris Malley [4:23 PM]\r\nYes, I read that. Your reason for feeling that `$h` needed to be ported literally?\r\n\r\n\r\nSam Reid [4:24 PM]\r\nI’m guessing the entire example was copied verbatim\r\n\r\n\r\nChris Malley [4:25 PM]\r\nWhere is the attribution for the code that you copied? (edited) \r\n\r\n`// Usage code from JS-Fourier-Image-Analysis/js/main.js`  ?\r\n\r\nAssuming that’s https://github.com/turbomaze/JS-Fourier-Image-Analysis… Is the license (MIT) compatible?\r\n\r\n\r\nSam Reid [4:28 PM]\r\nI’m seeing around 60 lines of code copied from https://github.com/turbomaze/JS-Fourier-Image-Analysis/blob/master/js/main.js\r\n</details>\r\n<br><br>\r\n\r\nThe delta in https://github.com/phetsims/wave-interference/commit/c98693b1c58d7136de589c0ffd1b6a023292b983 has `REVIEW` comments that are relevant:\r\n\r\n```js\r\n      //REVIEW while $h may have been in the copied code, consider using a more 'conventional' function name.\r\n      // store them in a nice function to match the math\r\n      //REVIEW* I'm not too familiar with this code.  If it survives to dev testing, I would like to factor out\r\n      //REVIEW* as much of this code to sherpa as I can.  For now, I renamed the function.\r\n      const lookupHHat = ( k, l ) => {\r\n        if ( arguments.length === 0 ) {\r\n          return h_hats;\r\n        }\r\n\r\n        const idx = k * dims[ 0 ] + l;\r\n        return h_hats[ idx ];\r\n      };\r\n```\r\n\r\nSo to summarize:\r\n\r\n- [ ] A chunk of code was copied from JS-Fourier-Image-Analysis/js/main.js\r\n- [ ] The code may or may not survive. If it does survive, the plan is to move it to sherpa.\r\n- [ ] Compatibility of the license (MIT) needs to be confirmed.\r\n- [ ] While this screen will not be visible in 1.0, you should determine whether this code will appear in the .html files (it looks like 'yes'), and whether doing so violates any licensing. "},"comment":{"url":"https://api.github.com/repos/phetsims/wave-interference/issues/comments/449456628","html_url":"https://github.com/phetsims/wave-interference/issues/278#issuecomment-449456628","issue_url":"https://api.github.com/repos/phetsims/wave-interference/issues/278","id":449456628,"node_id":"MDEyOklzc3VlQ29tbWVudDQ0OTQ1NjYyOA==","user":{"login":"samreid","id":679486,"node_id":"MDQ6VXNlcjY3OTQ4Ng==","avatar_url":"https://avatars0.githubusercontent.com/u/679486?v=4","gravatar_id":"","url":"https://api.github.com/users/samreid","html_url":"https://github.com/samreid","followers_url":"https://api.github.com/users/samreid/followers","following_url":"https://api.github.com/users/samreid/following{/other_user}","gists_url":"https://api.github.com/users/samreid/gists{/gist_id}","starred_url":"https://api.github.com/users/samreid/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/samreid/subscriptions","organizations_url":"https://api.github.com/users/samreid/orgs","repos_url":"https://api.github.com/users/samreid/repos","events_url":"https://api.github.com/users/samreid/events{/privacy}","received_events_url":"https://api.github.com/users/samreid/received_events","type":"User","site_admin":false},"created_at":"2018-12-21T17:59:38Z","updated_at":"2018-12-21T17:59:38Z","author_association":"MEMBER","body":"I stripped out DiffractionScreen so it no longer appears in builds.  I also asked on the slack channel:\r\n\r\n> Sam Reid [10:56 AM]\r\n> Will `Unused images file` or `Unused string:` fail an RC or production deploy?  It seems like just a warning for dev version.\r\n\r\nThis seems sufficient to defer for 2.0.  @pixelzoom if you concur, please reassign to me and add \"deferred\" label."}}
{"actor":{"display_login":"samreid"}}