Details

    • Testing Instructions:
      Hide

      These instructions assume that you have already installed jshint from node.js (npm -g install jshint)
      They also assume that you've downloaded https://github.com/yui/shifter/blob/master/.jshintrc and stick it in your home directory (or moodle checkout).

      In all supported browsers (IE, Chrome, Safari, Firefox):

      • Run jshint over lib/yui/formautosubmit/formautosubmit.js
        • Confirm that no errors were shown
      • Open a course page
      • Open your JS console
        • If possible, turn on recording so that messages aren't lost between page reloads
      • Turn editing on
      • Ensure that the activity chooser is off
      • On a random (not first, or last) 'Add a resource...' dropdown:
        • select an option
        • Confirm that the page starts to navigate away
        • Cancel the page reload (if possible)
        • Confirm that no errors or warnings were shown in the JS console
      • On a different dropdown, repeat the above (to make sure that it's not just working on one dropdown on the page
      • Repeat the above tests on the 'Add a block' dropdown (it's a different beast)
      • Open the /course/search.php page and find some results
      • Try using the 'Move selected courses to...' dropdown
        • Confirm that no errors were shown
        • Confirm that the page reloads and reflects the changes you made (or didn't make)
      Show
      These instructions assume that you have already installed jshint from node.js (npm -g install jshint) They also assume that you've downloaded https://github.com/yui/shifter/blob/master/.jshintrc and stick it in your home directory (or moodle checkout). In all supported browsers (IE, Chrome, Safari, Firefox): Run jshint over lib/yui/formautosubmit/formautosubmit.js Confirm that no errors were shown Open a course page Open your JS console If possible, turn on recording so that messages aren't lost between page reloads Turn editing on Ensure that the activity chooser is off On a random (not first, or last) 'Add a resource...' dropdown: select an option Confirm that the page starts to navigate away Cancel the page reload (if possible) Confirm that no errors or warnings were shown in the JS console On a different dropdown, repeat the above (to make sure that it's not just working on one dropdown on the page Repeat the above tests on the 'Add a block' dropdown (it's a different beast) Open the /course/search.php page and find some results Try using the 'Move selected courses to...' dropdown Confirm that no errors were shown Confirm that the page reloads and reflects the changes you made (or didn't make)
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-37366-master
    • Rank (Obsolete):
      47021

      Description

      As it says on the tin

        Activity

        Hide
        Damyon Wiese added a comment -

        Hi Andrew,

        Thanks for the patch.

        Issue 1.
        This has a similar issue to the other jslint issue:
        I don't like the indenting of this:

                // The CSS selectors we use
                var CSS = {
                    AUTOSUBMIT : 'autosubmit'
                },
        
                FORMAUTOSUBMITNAME = 'core-formautosubmit',
        
                FORMAUTOSUBMIT = function() {
                    FORMAUTOSUBMIT.superclass.constructor.apply(this, arguments);
                },
        
                //  We only want to initialize the module fully once
                INITIALIZED = false;
        

        Can you please fix this?

        Issue 2.
        The testing instructions could be a bit more specific and list browsers (recommend just Chrome, IE and Firefox).

        Issue 3. Nothing to do - but this is an improvement and should not be backported.

        Thanks, Damyon

        Show
        Damyon Wiese added a comment - Hi Andrew, Thanks for the patch. Issue 1. This has a similar issue to the other jslint issue: I don't like the indenting of this: // The CSS selectors we use var CSS = { AUTOSUBMIT : 'autosubmit' }, FORMAUTOSUBMITNAME = 'core-formautosubmit', FORMAUTOSUBMIT = function() { FORMAUTOSUBMIT.superclass.constructor.apply( this , arguments); }, // We only want to initialize the module fully once INITIALIZED = false ; Can you please fix this? Issue 2. The testing instructions could be a bit more specific and list browsers (recommend just Chrome, IE and Firefox). Issue 3. Nothing to do - but this is an improvement and should not be backported. Thanks, Damyon
        Hide
        Andrew Nicols added a comment -

        1: What part of the indenting don't you like? I guess we could indent the subsequent var definitions, though I worry that it would be harder to read.

        2: I'll try and improve them.

        3: I'm in two minds on this - yes it's an improvement, but some of these lint issues may affect browser compat. - I guess those which do should only backport the relevant parts?

        Show
        Andrew Nicols added a comment - 1: What part of the indenting don't you like? I guess we could indent the subsequent var definitions, though I worry that it would be harder to read. 2: I'll try and improve them. 3: I'm in two minds on this - yes it's an improvement, but some of these lint issues may affect browser compat. - I guess those which do should only backport the relevant parts?
        Hide
        Andrew Nicols added a comment -

        I've adjusted the patch to improve indenting.
        I felt that doing so in the obvious way made it harder to read, so I modified things slightly. This does still conform with our code standards, but also makes it easier to read.

        I've improved the testing instructions

        I've dropped the patch for 2.4 - I'd still appreciate an integrator's views on this though.

        Show
        Andrew Nicols added a comment - I've adjusted the patch to improve indenting. I felt that doing so in the obvious way made it harder to read, so I modified things slightly. This does still conform with our code standards, but also makes it easier to read. I've improved the testing instructions I've dropped the patch for 2.4 - I'd still appreciate an integrator's views on this though.
        Hide
        Andrew Nicols added a comment -

        Putting up for peer review again, as I'd quite like your thoughts on the indenting changes.

        Show
        Andrew Nicols added a comment - Putting up for peer review again, as I'd quite like your thoughts on the indenting changes.
        Hide
        Damyon Wiese added a comment -

        Thanks Andrew this looks great now!

        The indenting is much better and the testing instructions are good.

        Full Peer review checklist:

        [Y] Syntax
        [-] Output
        [Y] Whitespace
        [-] Language
        [-] Databases
        [Y] Testing
        [-] Security
        [-] Documentation
        [Y] Git
        [Y] Sanity check

        Sending for integration review.

        Show
        Damyon Wiese added a comment - Thanks Andrew this looks great now! The indenting is much better and the testing instructions are good. Full Peer review checklist: [Y] Syntax [-] Output [Y] Whitespace [-] Language [-] Databases [Y] Testing [-] Security [-] Documentation [Y] Git [Y] Sanity check Sending for integration review.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

        TIA and ciao

        Show
        Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
        Hide
        Andrew Nicols added a comment -

        Just rebasing this...

        Show
        Andrew Nicols added a comment - Just rebasing this...
        Hide
        Andrew Nicols added a comment -

        Rebased. This was conflicting with MDL-37393 so I've rebased it now that that issue has been integrated.

        Show
        Andrew Nicols added a comment - Rebased. This was conflicting with MDL-37393 so I've rebased it now that that issue has been integrated.
        Hide
        Dan Poltawski added a comment -

        Integrated, thanks Andrew

        Show
        Dan Poltawski added a comment - Integrated, thanks Andrew
        Hide
        Michael de Raadt added a comment -

        Thanks for introducing me to JS Linting.

        I should note that I was running this under Windows. I tried this with Node and within the editor Notepad++.

        Before your changes there were 16 warnings. Afterwards, using your configuration, there were none. I noted you have laxbreak set to true. With that off there was a warning about the line break at line 85 before the && operator.

        I'll run the functional tests now...

        Show
        Michael de Raadt added a comment - Thanks for introducing me to JS Linting. I should note that I was running this under Windows. I tried this with Node and within the editor Notepad++. Before your changes there were 16 warnings. Afterwards, using your configuration, there were none. I noted you have laxbreak set to true. With that off there was a warning about the line break at line 85 before the && operator. I'll run the functional tests now...
        Hide
        Dan Poltawski added a comment -

        Michael: this is related to MDL-37127, it might be interesting for you to comment on your experiences there.

        Show
        Dan Poltawski added a comment - Michael: this is related to MDL-37127 , it might be interesting for you to comment on your experiences there.
        Hide
        Michael de Raadt added a comment -

        Test result: Success!

        Tested in Master only.

        Functional tests run in FF18, IE10, Chrome23 and Safari5.

        Show
        Michael de Raadt added a comment - Test result: Success! Tested in Master only. Functional tests run in FF18, IE10, Chrome23 and Safari5.
        Hide
        Andrew Nicols added a comment -

        Thanks Michael,

        That's useful to know about (laxbreak). When looking at settings, I thought it was probably best to go with the same settings that the YUI project use when doing their JS linting - drifting too far away leads to madness.

        The laxbreak is a warning about code style. It basically says that it expects long statements to be broken by certain characters, and a ) is not one of those (not when the statement is not yet finished). The ''correct'' way of doing it according to jshint would be to end the line with something like a &&.

        Compare:

        if ((nothing===false || select.get('value') !== nothing)
                && startindex !== select.get('selectedIndex') && currentindex !== previousindex) {
        }
        

        With:

        if ((nothing===false || select.get('value') !== nothing) &&
                startindex !== select.get('selectedIndex') && currentindex !== previousindex) {
        }
        

        It makes absolutely no difference to the functionality of the code, but in my opinion, it's not immediately clear that the second line of the statement is a continuation at a first glance. Equally though I can see why it could be argued that the first line could appear complete when it isn't.

        In any case it's entirely a code readability issue and doesn't actually affect how the code is interpreted. We can disable the laxbreak exception and fix the small handful of issues it would create at this time - if we want to do so, then we should do so sooner rather than later.

        In any case, it would be good if you could comment in the issue Dan highlighted (MDL-37127) about how you found the process of installing node and jshint (I've not tried under Windows actually), and your other experiences.

        Andrew

        Show
        Andrew Nicols added a comment - Thanks Michael, That's useful to know about (laxbreak). When looking at settings, I thought it was probably best to go with the same settings that the YUI project use when doing their JS linting - drifting too far away leads to madness. The laxbreak is a warning about code style. It basically says that it expects long statements to be broken by certain characters, and a ) is not one of those (not when the statement is not yet finished). The ''correct'' way of doing it according to jshint would be to end the line with something like a &&. Compare: if ((nothing=== false || select.get('value') !== nothing) && startindex !== select.get('selectedIndex') && currentindex !== previousindex) { } With: if ((nothing=== false || select.get('value') !== nothing) && startindex !== select.get('selectedIndex') && currentindex !== previousindex) { } It makes absolutely no difference to the functionality of the code, but in my opinion, it's not immediately clear that the second line of the statement is a continuation at a first glance. Equally though I can see why it could be argued that the first line could appear complete when it isn't. In any case it's entirely a code readability issue and doesn't actually affect how the code is interpreted. We can disable the laxbreak exception and fix the small handful of issues it would create at this time - if we want to do so, then we should do so sooner rather than later. In any case, it would be good if you could comment in the issue Dan highlighted ( MDL-37127 ) about how you found the process of installing node and jshint (I've not tried under Windows actually), and your other experiences. Andrew
        Hide
        Dan Poltawski added a comment -

        Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

        Show
        Dan Poltawski added a comment - Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: