Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor 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:
    • Rank:
      48853

      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.

        Activity

        Hide
        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
        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
        Pierre Pichet added a comment -

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

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

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

        Show
        Tim Hunt added a comment - Given the "I will rework this tonigth. " comment, I am taking this out of peer review for now.
        Hide
        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
        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
        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
        Pierre Pichet added a comment -

        Done and push on github
        Thanks for the detailed review.

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

        +1 Submitting for integration.

        Show
        Tim Hunt added a comment - +1 Submitting for integration.
        Hide
        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
        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
        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
        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
        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
        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
        Pierre Pichet added a comment -

        Changes done

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

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

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

        Fair enough, integrated, thanks!

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

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

        Show
        Eloy Lafuente (stronk7) added a comment - Due the nature of the changes, nobody tested this. Passing.
        Hide
        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
        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: