Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.6, 2.4.3, 2.5
    • Fix Version/s: 2.5
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide

      This change should not change any behaviour, and we are about to have a full Moodle QA cycle. I think it should just be integrated, and I don't think any manual testing will really add any value.

      If you like, do a bit of sanity checking on the question funcitonality, to verify that nothing is obviously broken.

      Show
      This change should not change any behaviour, and we are about to have a full Moodle QA cycle. I think it should just be integrated, and I don't think any manual testing will really add any value. If you like, do a bit of sanity checking on the question funcitonality, to verify that nothing is obviously broken.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      MDL-31680 is an example the problem
      Eloy Lafuente comment:
      Also, plz, in the future don't mix the solution to a bug with dozens of style changes, it makes really difficult to analyze the solution.

        Gliffy Diagrams

          Activity

          Hide
          ppichet Pierre Pichet added a comment -

          Finaaly i pass through all files in question/type directories to correct all in-line comments so that they did not show in warnings apart from
          a more than 132 characters line.
          There should be no code correction to test although I did not do a php code test.

          Show
          ppichet Pierre Pichet added a comment - Finaaly i pass through all files in question/type directories to correct all in-line comments so that they did not show in warnings apart from a more than 132 characters line. There should be no code correction to test although I did not do a php code test.
          Hide
          ppichet Pierre Pichet added a comment -

          The compare shows that some corrections on multilines comment can be minimized.
          I will rework this tonigth.

          Show
          ppichet Pierre Pichet added a comment - The compare shows that some corrections on multilines comment can be minimized. I will rework this tonigth.
          Hide
          timhunt Tim Hunt added a comment -

          Given the "I will rework this tonigth. " comment, I am taking this out of peer review for now.

          Show
          timhunt Tim Hunt added a comment - Given the "I will rework this tonigth. " comment, I am taking this out of peer review for now.
          Hide
          ppichet Pierre Pichet added a comment -

          I think it can be applied to master as the changes are mostly related to in-line comments code checker restriction to use
          // Capital letter first syntax.

          Show
          ppichet Pierre Pichet added a comment - I think it can be applied to master as the changes are mostly related to in-line comments code checker restriction to use // Capital letter first syntax.
          Show
          timhunt Tim Hunt added a comment - Great, but just a few problems: 1. This looks bad: https://github.com/ppichet/moodle/compare/master...MDL-38792#L4R496 2. Extra space should be deleted: https://github.com/ppichet/moodle/compare/master...MDL-38792#L6R251 3. Same: https://github.com/ppichet/moodle/compare/master...MDL-38792#L9R597 4. I think you should just delete this commented-out code: https://github.com/ppichet/moodle/compare/master...MDL-38792#L12R159 5. https://github.com/ppichet/moodle/compare/master...MDL-38792#L18R156 6. https://github.com/ppichet/moodle/compare/master...MDL-38792#L34R138 still has 3 /s
          Hide
          ppichet Pierre Pichet added a comment -

          Done and push on github
          Thanks for the detailed review.

          Show
          ppichet Pierre Pichet added a comment - Done and push on github Thanks for the detailed review.
          Hide
          timhunt Tim Hunt added a comment -

          +1 Submitting for integration.

          Show
          timhunt Tim Hunt added a comment - +1 Submitting for integration.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment - - edited

          Well, there are some changes that I don't know if have sense, seem to be "too many splitting of, originally, natural phrases, for example:

          -                // calculated options
          -                // it cannot go here without having done the first page
          -                // so the question_calculated_options should exist
          -                // only need to update the synchronize field
          +                // Calculated options.
          +                // It cannot go here without having done the first page.
          +                // So the question_calculated_options should exist.
          +                // Only need to update the synchronize field.
          

          or

          -        // get the dataset definitions for this question
          -        // coming here everytime even when using a NoSubmitButton
          -        // so this will only set the values to the actual question database content
          -        // which is not what we want so this should be removed from here
          -        // get priority to paramdatasets
          +        // Get the dataset definitions for this question.
          +        // coming here everytime even when using a NoSubmitButton.
          +        // So this will only set the values to the actual question database content.
          +        // which is not what we want so this should be removed from here.
          +        // Get priority to paramdatasets.
          

          And also, the opposite in some way:

          -    // Returns false if everything is alright.
          -    // Otherwise it constructs an error message
          -    // Strip away dataset names
          +    // Returns false if everything is alright
          +    // otherwise it constructs an error message.
          +    // Strip away dataset names.
          

          Also, I've seen some trailing whitespace here and there. Those MUST be fixed.

          Finally, the:

          // ...-----------------
          

          thing... is really, really horrible. Are the dots needed, perhaps we should allow the "all-dashes" alternative?

          And that is all. Apart from the illegal whitespace that MUST be fixed, I've nothing against any of the other details, if that help you to continue with other code.

          So, holding this waiting for whitespace fixes & feedback about the others...

          List of codechecker problems in the lines you changed:

          http://stronk7.doesntexist.com/job/Precheck%20remote%20branch/325/artifact/work/smurf.xml

          List of codechecker problems in the files (whole) you changed:

          http://stronk7.doesntexist.com/view/prehecker/job/Precheck%20remote%20branch/325/artifact/work/cs.xml

          Ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - - edited Well, there are some changes that I don't know if have sense, seem to be "too many splitting of, originally, natural phrases, for example: - // calculated options - // it cannot go here without having done the first page - // so the question_calculated_options should exist - // only need to update the synchronize field + // Calculated options. + // It cannot go here without having done the first page. + // So the question_calculated_options should exist. + // Only need to update the synchronize field. or - // get the dataset definitions for this question - // coming here everytime even when using a NoSubmitButton - // so this will only set the values to the actual question database content - // which is not what we want so this should be removed from here - // get priority to paramdatasets + // Get the dataset definitions for this question. + // coming here everytime even when using a NoSubmitButton. + // So this will only set the values to the actual question database content. + // which is not what we want so this should be removed from here. + // Get priority to paramdatasets. And also, the opposite in some way: - // Returns false if everything is alright. - // Otherwise it constructs an error message - // Strip away dataset names + // Returns false if everything is alright + // otherwise it constructs an error message. + // Strip away dataset names. Also, I've seen some trailing whitespace here and there. Those MUST be fixed. Finally, the: // ...----------------- thing... is really, really horrible. Are the dots needed, perhaps we should allow the "all-dashes" alternative? And that is all. Apart from the illegal whitespace that MUST be fixed, I've nothing against any of the other details, if that help you to continue with other code. So, holding this waiting for whitespace fixes & feedback about the others... List of codechecker problems in the lines you changed: http://stronk7.doesntexist.com/job/Precheck%20remote%20branch/325/artifact/work/smurf.xml List of codechecker problems in the files (whole) you changed: http://stronk7.doesntexist.com/view/prehecker/job/Precheck%20remote%20branch/325/artifact/work/cs.xml Ciao
          Hide
          ppichet Pierre Pichet added a comment -

          Thanks for your additional comments.
          I will modify accordingly.

          Some comments were badly updated to the codechecker rule of first capital letter and finish by . or ?
          until I found that the rules are more flexible if the comments are in successive lines.
          Then the first line should begin by a capital letter and the last line finish by . or ?

          The code checker does not work well when as I do your file is in DOS CRLF format and the code is
          for example a database call that extends on multiple lines.

          My main objective was to eliminate code checker related to inline comments.

          I will notify when done.

          Show
          ppichet Pierre Pichet added a comment - Thanks for your additional comments. I will modify accordingly. Some comments were badly updated to the codechecker rule of first capital letter and finish by . or ? until I found that the rules are more flexible if the comments are in successive lines. Then the first line should begin by a capital letter and the last line finish by . or ? The code checker does not work well when as I do your file is in DOS CRLF format and the code is for example a database call that extends on multiple lines. My main objective was to eliminate code checker related to inline comments. I will notify when done.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          I'm reopening it in the mean time, please send it back to integration when ready. We are under continuous integration and hopefully will be picked quickly.

          Thanks!

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - I'm reopening it in the mean time, please send it back to integration when ready. We are under continuous integration and hopefully will be picked quickly. Thanks!
          Hide
          ppichet Pierre Pichet added a comment -

          Changes done

          Show
          ppichet Pierre Pichet added a comment - Changes done
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          oh, lol, that was quick! looking...

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - oh, lol, that was quick! looking...
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Fair enough, integrated, thanks!

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Fair enough, integrated, thanks!
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Due the nature of the changes, nobody tested this. Passing.

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Due the nature of the changes, nobody tested this. Passing.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Your awesome contributions are now part of Moodle, your fav LMS out there.

          Closing this as fixed.

          Many thanks for all the hard work, ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Your awesome contributions are now part of Moodle, your fav LMS out there. Closing this as fixed. Many thanks for all the hard work, ciao

            People

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

              Dates

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