Moodle

Updating question/type files in-line comments to code checker standards

Details

  • Type: Sub-task Sub-task
  • Status: Closed 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:
  • Pull Master Diff URL:

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

Ascending order - Click to sort in descending order
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
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.
Hide
Permalink
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

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
Permalink
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
Permalink
Tim Hunt added a comment -

+1 Submitting for integration.

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

Changes done

Show
Pierre Pichet added a comment - Changes done
Hide
Permalink
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
Permalink
Eloy Lafuente (stronk7) added a comment -

Fair enough, integrated, thanks!

Show
Eloy Lafuente (stronk7) added a comment - Fair enough, integrated, thanks!
Hide
Permalink
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
Permalink
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

Vote (1)
Watch (3)

Dates

  • Created:
    Updated:
    Resolved: