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:
-
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:
-
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
The compare shows that some corrections on multilines comment can be minimized.
I will rework this tonigth.
Given the "I will rework this tonigth. " comment, I am taking this out of peer review for now.
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.
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
Done and push on github
Thanks for the detailed review.
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:
Ciao
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.
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!
Changes done
oh, lol, that was quick! looking...
Fair enough, integrated, thanks!
Due the nature of the changes, nobody tested this. Passing.
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
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.