Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3, 2.3.4, 2.4.1
    • Fix Version/s: 2.5
    • Component/s: JavaScript
    • Labels:
    • Testing Instructions:
      Hide

      Run jshint (available from Node.js) (npm -g install jshint)over lib/yui/formchangechecker/formchangecheck.js for each branch

      In Firefox, Chrome and IE browsers:

      • Open the course settings page (form)
      • Refresh the page
        • Confirm that no warnings were shown
      • Enter some content
      • Refresh the page
        • Confirm that a warnings was shown before navigating away
      Show
      Run jshint (available from Node.js) (npm -g install jshint)over lib/yui/formchangechecker/formchangecheck.js for each branch In Firefox, Chrome and IE browsers: Open the course settings page (form) Refresh the page Confirm that no warnings were shown Enter some content Refresh the page Confirm that a warnings was shown before navigating away
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:

      Description

      Fix JS Hint with the form change checker

        Gliffy Diagrams

          Attachments

            Activity

            Hide
            damyon Damyon Wiese added a comment -

            Hi Andrew,

            Thanks for submitting this the improvements make sense to me.

            I have one issue though - when declaring multiple variables with one var statement, the subsequent variables should be indented for readability.

            e.g.

            +                    var formid = 'form#' + this.get('formid'),
            +                    currentform = Y.one(formid);

            should be:

            +                    var formid = 'form#' + this.get('formid'),
            +                        currentform = Y.one(formid);

            Also - this is an improvement IMO and should not be backported.

            Show
            damyon Damyon Wiese added a comment - Hi Andrew, Thanks for submitting this the improvements make sense to me. I have one issue though - when declaring multiple variables with one var statement, the subsequent variables should be indented for readability. e.g. + var formid = 'form#' + this.get('formid'), + currentform = Y.one(formid); should be: + var formid = 'form#' + this.get('formid'), + currentform = Y.one(formid); Also - this is an improvement IMO and should not be backported.
            Hide
            damyon Damyon Wiese added a comment - - edited

            Note: There is more than one instance of that indenting issue - I only listed one.

            Show
            damyon Damyon Wiese added a comment - - edited Note: There is more than one instance of that indenting issue - I only listed one.
            Hide
            damyon Damyon Wiese added a comment -

            Just adding a subset of the browsers to the testing instructions as this is a low risk change.

            Show
            damyon Damyon Wiese added a comment - Just adding a subset of the browsers to the testing instructions as this is a low risk change.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Thanks Damyon,

            Adjusted as per your suggestions (indenting) and dropping the other branches.

            Submitting for IR.

            Show
            dobedobedoh Andrew Nicols added a comment - Thanks Damyon, Adjusted as per your suggestions (indenting) and dropping the other branches. Submitting for IR.
            Hide
            poltawski Dan Poltawski 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
            poltawski Dan Poltawski 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
            poltawski Dan Poltawski added a comment -

            Integrated thanks Andrew.

            Show
            poltawski Dan Poltawski added a comment - Integrated thanks Andrew.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Ankit, you may wish to see MDL-37626 for a suitable jshint configuration.

            Show
            dobedobedoh Andrew Nicols added a comment - Ankit, you may wish to see MDL-37626 for a suitable jshint configuration.
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Thanks Andrew, Just got jshint working after looking at details in a related issue.

            Show
            ankit_frenz Ankit Agarwal added a comment - Thanks Andrew, Just got jshint working after looking at details in a related issue.
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Andrew,
            You want a strict mode check right?
            I am getting following error with strict mode:-

            lib/yui/formchangechecker/formchangechecker.js: line 3, col 9, Missing "use strict" statement.
             
            1 error

            No errors with strict mode being false.

            Please suggest if tests needs to be failed.
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - Andrew, You want a strict mode check right? I am getting following error with strict mode:- lib/yui/formchangechecker/formchangechecker.js: line 3, col 9, Missing "use strict" statement.   1 error No errors with strict mode being false. Please suggest if tests needs to be failed. Thanks
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Hi Ankit,

            No we're not enforcing strict at present, and I don't think that we're likely to either. Strict can be quite difficult to get right.
            To support it fully, we'd have to add a use strict to every function, and this could more breakages than it would solve.

            At the moment, I'm suggesting we use the jshintrc used by the YUI project as this will help us to get our modules to the same kind of standard as theirs.

            Andrew

            Show
            dobedobedoh Andrew Nicols added a comment - Hi Ankit, No we're not enforcing strict at present, and I don't think that we're likely to either. Strict can be quite difficult to get right. To support it fully, we'd have to add a use strict to every function, and this could more breakages than it would solve. At the moment, I'm suggesting we use the jshintrc used by the YUI project as this will help us to get our modules to the same kind of standard as theirs. Andrew
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            In that case, All is good.
            Passing
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - In that case, All is good. Passing Thanks
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Surely you will be happy to know that your code is now part of Moodle upstream. Thanks, thanks!

            Closing as fixed, ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Surely you will be happy to know that your code is now part of Moodle upstream. Thanks, thanks! Closing as fixed, ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  14/May/13