{"action":"created","issue":{"url":"https://api.github.com/repos/OHDSI/Atlas/issues/258","repository_url":"https://api.github.com/repos/OHDSI/Atlas","labels_url":"https://api.github.com/repos/OHDSI/Atlas/issues/258/labels{/name}","comments_url":"https://api.github.com/repos/OHDSI/Atlas/issues/258/comments","events_url":"https://api.github.com/repos/OHDSI/Atlas/issues/258/events","html_url":"https://github.com/OHDSI/Atlas/pull/258","id":182373105,"number":258,"title":"trying to use linters and fixing achilles","user":{"login":"fdefalco","id":6586384,"avatar_url":"https://avatars.githubusercontent.com/u/6586384?v=3","gravatar_id":"","url":"https://api.github.com/users/fdefalco","html_url":"https://github.com/fdefalco","followers_url":"https://api.github.com/users/fdefalco/followers","following_url":"https://api.github.com/users/fdefalco/following{/other_user}","gists_url":"https://api.github.com/users/fdefalco/gists{/gist_id}","starred_url":"https://api.github.com/users/fdefalco/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/fdefalco/subscriptions","organizations_url":"https://api.github.com/users/fdefalco/orgs","repos_url":"https://api.github.com/users/fdefalco/repos","events_url":"https://api.github.com/users/fdefalco/events{/privacy}","received_events_url":"https://api.github.com/users/fdefalco/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":8,"created_at":"2016-10-11T21:00:23Z","updated_at":"2016-10-12T15:46:23Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/OHDSI/Atlas/pulls/258","html_url":"https://github.com/OHDSI/Atlas/pull/258","diff_url":"https://github.com/OHDSI/Atlas/pull/258.diff","patch_url":"https://github.com/OHDSI/Atlas/pull/258.patch"},"body":"It turns out the issues we were seeing with the legacy achilles (data sources) charts was due to some undeclared variables throwing errors once \"use strict\" was applied on the charting module.  I've put a few things into this branch that I wanted to open up for comment before pushing to master.\r\n\r\nI added .jshintrc which is a preference file for JSHint.  JSHint is a javascript code analysis tool.  The .jshintrc file will allow us to set conventions for our code that we can then test for compliance.  The current .jshintrc is by no means \"our accepted standards\" just a template I found that seemed well formed.  I then made changes to bring the charting module file down from a few hundred JSHint problems to 39.  I stopped there as the problems were getting harder to resolve and I wasn't sure if this was a path we would want to adopt.\r\n\r\nIf we are in agreement and get approval on this branch then we can use JSHint (with extensions for brackets, vim, emacs, visual studio, notepad++ and other editors) going forward and make progress on \"house cleaning\" of our javascript code as time allows.  While this doesn't address the broader refactoring we need to do it should help get us moving in the right direction and give us a way to hold ourselves accountable to some quality standard.\r\n\r\nThis branch does fix the data sources modules and reports are now working so I've re-enabled it on the left menu.  I also made some style updates to the dropdown buttons for reports and data sources to fix  the display issues with the buttons.\r\n\r\nI also changed so that \"use strict\" is occurring at a function level instead of module as that was the approach I've seen recommended."},"comment":{"url":"https://api.github.com/repos/OHDSI/Atlas/issues/comments/253252460","html_url":"https://github.com/OHDSI/Atlas/pull/258#issuecomment-253252460","issue_url":"https://api.github.com/repos/OHDSI/Atlas/issues/258","id":253252460,"user":{"login":"chrisknoll","id":6818777,"avatar_url":"https://avatars.githubusercontent.com/u/6818777?v=3","gravatar_id":"","url":"https://api.github.com/users/chrisknoll","html_url":"https://github.com/chrisknoll","followers_url":"https://api.github.com/users/chrisknoll/followers","following_url":"https://api.github.com/users/chrisknoll/following{/other_user}","gists_url":"https://api.github.com/users/chrisknoll/gists{/gist_id}","starred_url":"https://api.github.com/users/chrisknoll/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/chrisknoll/subscriptions","organizations_url":"https://api.github.com/users/chrisknoll/orgs","repos_url":"https://api.github.com/users/chrisknoll/repos","events_url":"https://api.github.com/users/chrisknoll/events{/privacy}","received_events_url":"https://api.github.com/users/chrisknoll/received_events","type":"User","site_admin":false},"created_at":"2016-10-12T15:46:23Z","updated_at":"2016-10-12T15:46:23Z","body":"Several things here, I'll try to cover them one at a time:\r\n\r\n1. Thanks for clarifying, I found those added 'use strict' lines to the local functions. Moreover, we shouldn't be using 'use strict' at a global script level for reasons described here:\r\nhttps://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode\r\nSo, your change addresses two problems, so that's good.\r\n\r\n2. I think you're conflating the issues of refactoring, adding linting and fixing an issue into a single PR as the encouraged activity.  Factoring is good,  Bug fixing is good.  Adopting better practices is good.  Putting all of those things into one PR?  Not good because now we have to address all those elements as part of a singe PR review.  If i were to fix a bug by introducing a testing framework and build system, I think you'd agree that a single PR that fixes a bug, introduces a test framework and introduces a build system would probably be too much for a single PR.  instead, separate PRs for those things would be preferred so that each one can be evaluated in isolation (unless the bug with Achilles will remain broken without the linting configuration changes, but that's not the case here). So, we'll have to deal with the linting configuration part of the PR which leads us to...\r\n\r\n3. I don't think the .jshintrc file you've provided should be used as is. I could either add comments into the commuted file directly, or call it out here, not sure which would be more effective.  Here's the items I think should be changed:\r\n\r\n\"eqeqeq\": sounds good to always use === for comparison, but there's a special case described here: http://stackoverflow.com/questions/359494/which-equals-operator-vs-should-be-used-in-javascript-comparisons.  I'm not sure of the case where we get a value from a AJAX call from the server and we want to compare a string value to a value from the AJAX, will === return false because they aren't referencing the same string reference?  Testing is required for this.\r\n\r\n\"plusplus\": I don't think using for (var i = 0; i< 20; i++) should be a linting error because of the ++ use.\r\n \r\n\"smarttabs\":  I agree with the setting of 'false' but we need to all get into alignment as to how to indent/format our code.  Many commits are in the repo with lines changed where it was a tab-to-space or space-to-tab or -number of tabs in an indent problem.  \r\n\r\n\"esversion\": I'm not sure 6 is what we want here, the docs give the options of 3, 5 and 6: http://jshint.com/docs/options/.  An example of an ES6 feature that caused us problems recently is here: http://forums.ohdsi.org/t/atlas-on-tomcat-works-from-remote-host-but-not-localhost/1741/9\r\n\r\n\"devel\": I think this should be false, I think people should be warned about leaving console.log messages in the final code, since it probably represents debugging and not actual functionality.  And the editor would be able to show you where that is happening.\r\n\r\nFormatting and Complexity: I agree most of these are things that really depend on the complexity of the code.  So having a hard rule about \"no more than 10 function params\" is not something I want to see. However, there's one value that is worth discussing:\r\n\r\n\"indent\": This item is actually going to be removed from jslint due to \"JSHint is limiting its scope to issues of code correctness. If you would like to enforce rules relating to code style, check out the JSCS project\"  So, perhaps the issues raised above about code style and formatting is actually beyond scope of this PR but it is definitely something we all need to come to agreement over.\r\n\r\nAgain, this is a good idea and i'm not in any way pushing back against having linting in code.  I just want to make sure that we get as close to the 'accepted settings' as possible before we release it out to the general public.\r\n\r\n\r\n\r\n"}}
{"actor":{"display_login":"chrisknoll"}}