Details

    • Testing Instructions:
      Hide

      Install node.js and then install jshint (npm install -g jshint).

      • Try passing a few files through jshint:
        1. lib/yui/chooserdialogue/chooserdialogue.js
        2. lib/yui/formautosubmit/formautosubmit.js
        3. lib/yui/formchangechecker/formchangechecker.js
      • Open up the config and change the "strict" option to true
      • Rerun the tests
      • Confirm that some errors were shown this time
      • Try through some files we haven't yet cleaned up:
        1. lib/yui/notification/notification.js
      Show
      Install node.js and then install jshint (npm install -g jshint). Try passing a few files through jshint: lib/yui/chooserdialogue/chooserdialogue.js lib/yui/formautosubmit/formautosubmit.js lib/yui/formchangechecker/formchangechecker.js Open up the config and change the "strict" option to true Rerun the tests Confirm that some errors were shown this time Try through some files we haven't yet cleaned up: lib/yui/notification/notification.js
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
    • Rank:
      47317

      Description

      It would be particularly good if we could add the .jshintrc to moodle.git
      I know that we haven't 100% finalised on a tool yet, but at present, I'm trying to encourage all changed and new JS to be jshinted and this would make our life easier.

      I suggest that we start with the jshint settings used by YUI: https://raw.github.com/yui/yui-lint/master/jshint.json

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment -

          I think
          "onevar": true,
          is stupid. Sufficiently stupid that we should differ from YUI there.

          The options are explained here: http://www.jshint.com/docs/

          Show
          Tim Hunt added a comment - I think "onevar": true, is stupid. Sufficiently stupid that we should differ from YUI there. The options are explained here: http://www.jshint.com/docs/
          Hide
          Dan Poltawski added a comment -

          I think "onevar": true, is stupid.

          Agreed. +1 to differ

          Show
          Dan Poltawski added a comment - I think "onevar": true, is stupid. Agreed. +1 to differ
          Hide
          Dan Poltawski added a comment -

          Its fine for me, I would just comment that two options I wasn't 100% certain about:

          node (is this necessary for the new proposed build process?)
          eqeqeq (we are not that strict with php)

          It might be interesting to run on the code base and see if there are things you've switched on there which trigger problems everywhere (even on relatievly modern and good code)

          Show
          Dan Poltawski added a comment - Its fine for me, I would just comment that two options I wasn't 100% certain about: node (is this necessary for the new proposed build process?) eqeqeq (we are not that strict with php) It might be interesting to run on the code base and see if there are things you've switched on there which trigger problems everywhere (even on relatievly modern and good code)
          Hide
          Andrew Nicols added a comment -

          Thanks Dan,

          I agree with onevar and have made that change.

          I'm less inclined to agree about eqeqeq though – as you say, we don't strictly apply it in our php code, but I don't think that that's an argument to not be stricter in JavaScript.

          JS coercion can be really helpful, but it can also be really confusing and give some really odd results in tests. I'm not saying that it can't be useful in comparisons (because it can), and I'm not saying that we should never use it in our JS. In fact the jshint documentation links to a page (http://javascriptweblog.wordpress.com/2011/02/07/truth-equality-and-javascript/) detailing how it's not evil. But given the number of people working on Moodle, there is a greater chance of coercion mistakes occurring.

          I think that we should aim to use strict comparisons. In cases where a non-strict comparison would be better, it is possible to override the jshint configuration for a specific function. This also serves as a note to those coming along in the future to perhaps think about the possible complications.

          Here's an excerpt from the jshint documentation:

          These options tell JSHint to be more strict towards your code. Use them if you want to allow only a safe subset of JavaScript—very useful when your codebase is shared with a big number of developers with different skill levels.
          

          Additionally, if we start off with this option enabled, and we find it to be unhelpful and a nuisance, we can turn it off in the knowledge that the code passing lint to date is fine. If we start off and turn it on, then we may end up having to reconsider various code.

          I've updated the commit with the onevar change. If you feel that that's sufficient, can you submit for integration for me (I'm away on Monday)?

          Cheers,
          Andrew

          Show
          Andrew Nicols added a comment - Thanks Dan, I agree with onevar and have made that change. I'm less inclined to agree about eqeqeq though – as you say, we don't strictly apply it in our php code, but I don't think that that's an argument to not be stricter in JavaScript. JS coercion can be really helpful, but it can also be really confusing and give some really odd results in tests. I'm not saying that it can't be useful in comparisons (because it can), and I'm not saying that we should never use it in our JS. In fact the jshint documentation links to a page ( http://javascriptweblog.wordpress.com/2011/02/07/truth-equality-and-javascript/ ) detailing how it's not evil. But given the number of people working on Moodle, there is a greater chance of coercion mistakes occurring. I think that we should aim to use strict comparisons. In cases where a non-strict comparison would be better, it is possible to override the jshint configuration for a specific function. This also serves as a note to those coming along in the future to perhaps think about the possible complications. Here's an excerpt from the jshint documentation: These options tell JSHint to be more strict towards your code. Use them if you want to allow only a safe subset of JavaScript—very useful when your codebase is shared with a big number of developers with different skill levels. Additionally, if we start off with this option enabled, and we find it to be unhelpful and a nuisance, we can turn it off in the knowledge that the code passing lint to date is fine. If we start off and turn it on, then we may end up having to reconsider various code. I've updated the commit with the onevar change. If you feel that that's sufficient, can you submit for integration for me (I'm away on Monday)? Cheers, Andrew
          Hide
          Dan Poltawski added a comment -

          I'm fine with them. You didn't explain node though.

          (Still, i've submitted for integration, i'm fine with it so long as you justify it)

          Show
          Dan Poltawski added a comment - I'm fine with them. You didn't explain node though. (Still, i've submitted for integration, i'm fine with it so long as you justify it)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Do we really want this into core? Wouldn't be better to put it under some local_jshint plugin (much like the codechecker), with node & jshint embedded and allow people to run it from there?

          Just a thought...ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Do we really want this into core? Wouldn't be better to put it under some local_jshint plugin (much like the codechecker), with node & jshint embedded and allow people to run it from there? Just a thought...ciao
          Hide
          Andrew Nicols added a comment -

          Well, it could go in a local plugin – you would then have to specify the path to the config file, but that's not the end of the world.

          Unfortunately, node itself has to be compiled from source as it provides a complete JS engine (IIRC it's the complete Chrome JS engine which is written in C++) so including node probably wouldn't be a viable option without somehow providing binaries for every major OS.

          I think that jshint could be, but I'm not 100% sure if it can, or how that would work.

          Rather than as a local plugin, we could create a composer package containing the relevant configuration for jshint, plus a wrapper installed in vendor/bin/jshint which specified that configuration.

          Thoughts?

          Show
          Andrew Nicols added a comment - Well, it could go in a local plugin – you would then have to specify the path to the config file, but that's not the end of the world. Unfortunately, node itself has to be compiled from source as it provides a complete JS engine (IIRC it's the complete Chrome JS engine which is written in C++) so including node probably wouldn't be a viable option without somehow providing binaries for every major OS. I think that jshint could be, but I'm not 100% sure if it can, or how that would work. Rather than as a local plugin, we could create a composer package containing the relevant configuration for jshint, plus a wrapper installed in vendor/bin/jshint which specified that configuration. Thoughts?
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Lol, I thought node was 100% js (here we go, epic fail).

          I just was interested about having it under local because of its (functional) similarities with the code-checker, but feel free. Although the less we pollute core, the better (just that).

          In fact, I don't like the latest "behat.yml.dist", "phpunit.xml", "composer.json", "mdeploy.xxx" files being in core @ main dir at all.. just a personal feeling.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Lol, I thought node was 100% js (here we go, epic fail). I just was interested about having it under local because of its (functional) similarities with the code-checker, but feel free. Although the less we pollute core, the better (just that). In fact, I don't like the latest "behat.yml.dist", "phpunit.xml", "composer.json", "mdeploy.xxx" files being in core @ main dir at all.. just a personal feeling. Ciao
          Hide
          Damyon Wiese added a comment -

          I agree with all the options except for "node" as well. I don't have any better suggestions for where to keep this file and it fits in the same basket as phpunit.xml and friends so I guess the location is ok.

          I added dev_docs_required - if we are including this file we should recommend developers start using it when changing/adding javascript.

          Show
          Damyon Wiese added a comment - I agree with all the options except for "node" as well. I don't have any better suggestions for where to keep this file and it fits in the same basket as phpunit.xml and friends so I guess the location is ok. I added dev_docs_required - if we are including this file we should recommend developers start using it when changing/adding javascript.
          Hide
          Damyon Wiese added a comment -

          Everything else is fine - and having node enabled does not make much difference (we shouldn't be using node globals anyway) so I'll push this as is. If you can add a comment on this here Andrew we can turn it off if required in a new issue.

          Show
          Damyon Wiese added a comment - Everything else is fine - and having node enabled does not make much difference (we shouldn't be using node globals anyway) so I'll push this as is. If you can add a comment on this here Andrew we can turn it off if required in a new issue.
          Hide
          Damyon Wiese added a comment -

          Thanks Andrew, I have added this to master only.

          If you can confirm that we don't need node enabled, we can add a new issue for it.

          Show
          Damyon Wiese added a comment - Thanks Andrew, I have added this to master only. If you can confirm that we don't need node enabled, we can add a new issue for it.
          Hide
          Andrew Nicols added a comment -

          We don't need node enabled. The reason that I hadn't changed it was that (I forgot about it, and) I was trying to keep the jshintrc as close to the one used in the YUI project as possible. It does makes sense though with node to drop it until such time as we do suddenly have a node module for Moodle (if ever).

          Andrew

          Show
          Andrew Nicols added a comment - We don't need node enabled. The reason that I hadn't changed it was that (I forgot about it, and) I was trying to keep the jshintrc as close to the one used in the YUI project as possible. It does makes sense though with node to drop it until such time as we do suddenly have a node module for Moodle (if ever). Andrew
          Hide
          Andrew Nicols added a comment -

          Created MDL-37872

          Show
          Andrew Nicols added a comment - Created MDL-37872
          Hide
          David Monllaó added a comment -

          It passes. It could be good to be more specific about that is the expected output in the testing instructions.

          Show
          David Monllaó added a comment - It passes. It could be good to be more specific about that is the expected output in the testing instructions.
          Hide
          Damyon Wiese added a comment -

          Congratulations! This issue has been resolved. Thanks for helping to make Moodle better for everyone!

          Regards, Damyon

          Show
          Damyon Wiese added a comment - Congratulations! This issue has been resolved. Thanks for helping to make Moodle better for everyone! Regards, Damyon

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: