Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-39404

Update question/format files to match codechecker standards

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.6
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide

      Testing questions import

      Import the various sample files that are in question/format/xxx/tests/fixtures and verify that no warning or error is displayed during import with debugging set to DEVELOPER level.
      Some remarks:

      • the learnwise-example.xml sample file for learnwise format is not in tests/fixtures subdir (maybe we should fix that for consistency some day ?)
      • all sample files for Moodle xml format contains images and sounds, so they will produce "Before calling format_text(), the content must be processed with file_rewrite_pluginfile_urls()" warning, this is unrelated to this issue and MDL-39507 will take care of that problem, so ignore it for now.
      • there is no sample file for xhml format because this is an export only format

      Testing questions export

      For the formats that support export (Gift, Moodle XML, XHML) verify that you can export the questions in a category and that the exported file looks correct (for Gift and Moodle XML an easy way to do that is to try to re-import the file, and for XHML that the file only contains valid HML).

      Show
      Testing questions import Import the various sample files that are in question/format/xxx/tests/fixtures and verify that no warning or error is displayed during import with debugging set to DEVELOPER level. Some remarks: the learnwise-example.xml sample file for learnwise format is not in tests/fixtures subdir (maybe we should fix that for consistency some day ?) all sample files for Moodle xml format contains images and sounds, so they will produce "Before calling format_text(), the content must be processed with file_rewrite_pluginfile_urls()" warning, this is unrelated to this issue and MDL-39507 will take care of that problem, so ignore it for now. there is no sample file for xhml format because this is an export only format Testing questions export For the formats that support export (Gift, Moodle XML, XHML) verify that you can export the questions in a category and that the exported file looks correct (for Gift and Moodle XML an easy way to do that is to try to re-import the file, and for XHML that the file only contains valid HML).
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull Master Branch:

      Description

      Maybe this is not the right timing to do that just before a release but I failed to find some time to do it sooner.
      After Pierre Pichet's good work in MDL-31680 to clean a lot of style issues in question/type, I think I need to do the same for question/format.
      Fortunately a lot of files there are already compliant, the most offending one is gift/format.php and I have already done part of the job while working on the contributed giftmedia format so I just need to port these changes and fix the remaining.
      If such a change seems to risky for 2.5, jst hold it for 2.5.1

        Gliffy Diagrams

          Activity

          Hide
          jmvedrine Jean-Michel Vedrine added a comment -

          gift format is done except one "error" that I think is wrongly reported by codechecker.
          Still more work needed for other formats.

          Show
          jmvedrine Jean-Michel Vedrine added a comment - gift format is done except one "error" that I think is wrongly reported by codechecker. Still more work needed for other formats.
          Hide
          timhunt Tim Hunt added a comment -

          Certainly worth doing. whether it goes into 2.5, 2.5.1 or just 2.6.

          Note, however, that you may get merge conflicts when MDL-39230 is integrated, and that has to take priority.

          Show
          timhunt Tim Hunt added a comment - Certainly worth doing. whether it goes into 2.5, 2.5.1 or just 2.6. Note, however, that you may get merge conflicts when MDL-39230 is integrated, and that has to take priority.
          Hide
          jmvedrine Jean-Michel Vedrine added a comment -

          Yes, exactly, my intention was first to make a branch for for the changes in question/format/xml/format to solve Jamie's problem with importing combined questions but as this is only one part of MDL-39230 I don't know if I should create a new bugtracker issue or if you prefer I submit it in MDL-39230 ?
          Also I am unsure if I should fix webct styles issues in MDL-39404 as I have already done the same work in MDL-30001

          Show
          jmvedrine Jean-Michel Vedrine added a comment - Yes, exactly, my intention was first to make a branch for for the changes in question/format/xml/format to solve Jamie's problem with importing combined questions but as this is only one part of MDL-39230 I don't know if I should create a new bugtracker issue or if you prefer I submit it in MDL-39230 ? Also I am unsure if I should fix webct styles issues in MDL-39404 as I have already done the same work in MDL-30001
          Hide
          jmvedrine Jean-Michel Vedrine added a comment -

          I finally decided it is best to do webct format styles fixes here and other fixes in MDL-30001.
          We are now down to 4 errors and 17 warnings which I find pretty good.
          things left are

          • lines too long
          • one "error" that I think is wrongly reported
          • things I don't know how to handle

          Note: The code checker find an error in the line

          ·$expout·.=·"\$CATEGORY:·$question->category\n";

          error message: : Member variable "CATEGORY" must be all lower-case
          Do you think I should create an issue in CONTRIB for that ?

          Show
          jmvedrine Jean-Michel Vedrine added a comment - I finally decided it is best to do webct format styles fixes here and other fixes in MDL-30001 . We are now down to 4 errors and 17 warnings which I find pretty good. things left are lines too long one "error" that I think is wrongly reported things I don't know how to handle Note: The code checker find an error in the line ·$expout·.=·"\$CATEGORY:·$question->category\n"; error message: : Member variable "CATEGORY" must be all lower-case Do you think I should create an issue in CONTRIB for that ?
          Hide
          timhunt Tim Hunt added a comment -

          That $CATEGORY thing is definitely a codechecker bug! Please create a CONTRIB issue for it.

          Show
          timhunt Tim Hunt added a comment - That $CATEGORY thing is definitely a codechecker bug! Please create a CONTRIB issue for it.
          Hide
          jmvedrine Jean-Michel Vedrine added a comment -

          Now that MDL-39230 is closed I have rebased this one.
          As I said there is still 4 errors and 17 warnings reported by the codechecker.

          Show
          jmvedrine Jean-Michel Vedrine added a comment - Now that MDL-39230 is closed I have rebased this one. As I said there is still 4 errors and 17 warnings reported by the codechecker.
          Hide
          timhunt Tim Hunt added a comment -

          Are you asking for peer review, or are you asking me to look at the remaining failures and comment on whether any more can/should be fixed?

          Show
          timhunt Tim Hunt added a comment - Are you asking for peer review, or are you asking me to look at the remaining failures and comment on whether any more can/should be fixed?
          Hide
          jmvedrine Jean-Michel Vedrine added a comment -

          In fact you don't need to look at the remaining failures because they all fall in a very small number of cases so I just need your advice on these cases and it will be faster if I expose these cases to you here:

          1. what to do with a non conformant comment like //·qtype_numerical::UNITNONE; see line 674 of question\format\xml\format.php
          2. what to do with a way too long line in tests like

            <text><![CDATA[Complete·this·opening·line·of·verse:·"The·{1:SHORTANSWER:Dog#Wrong,·silly!~=Owl#Well·done!~*#Wrong·answer}·and·the·{1:MULTICHOICE:Bow-wow#You·seem·to·have·a·dog·obsessions!~Wiggly·worm#Now·you·are·just·being·ridiculous!~=Pussy-cat#Well·done!}·went·to·sea".]]></text>

            see line 1289 of question\format\xml\tests\xmlformat_test.php. IMHO breaking it don't improve readability at all but I guess this is just a personal opinion.

          3. what to do with commented code like

            //·$question·=·$this->defaultquestion();

            see line 189 of question\format\webct\format.php I think it must be suppressed ?

          Show
          jmvedrine Jean-Michel Vedrine added a comment - In fact you don't need to look at the remaining failures because they all fall in a very small number of cases so I just need your advice on these cases and it will be faster if I expose these cases to you here: what to do with a non conformant comment like //·qtype_numerical::UNITNONE; see line 674 of question\format\xml\format.php what to do with a way too long line in tests like <text><![CDATA[Complete·this·opening·line·of·verse:·"The·{1:SHORTANSWER:Dog#Wrong,·silly!~=Owl#Well·done!~*#Wrong·answer}·and·the·{1:MULTICHOICE:Bow-wow#You·seem·to·have·a·dog·obsessions!~Wiggly·worm#Now·you·are·just·being·ridiculous!~=Pussy-cat#Well·done!}·went·to·sea".]]></text> see line 1289 of question\format\xml\tests\xmlformat_test.php. IMHO breaking it don't improve readability at all but I guess this is just a personal opinion. what to do with commented code like //·$question·=·$this->defaultquestion(); see line 189 of question\format\webct\format.php I think it must be suppressed ?
          Hide
          timhunt Tim Hunt added a comment -

          1. Convert this to a sentence: e.g.

          //·This is qtype_numerical::UNITNONE, but we cannot refer to that constant here.

          2. Leave that one as it is. (Or you could line wrap it, but as you say, that probably makes things worse.)

          3. Just delete commented out code like that. If anyone needs it, it is in git. If there is a good reason to keep it, then add words to the comment, explaining why, which should make codechecker stop complaining. Or, if the worst comes to worst, leave that as is.

          Show
          timhunt Tim Hunt added a comment - 1. Convert this to a sentence: e.g. //·This is qtype_numerical::UNITNONE, but we cannot refer to that constant here. 2. Leave that one as it is. (Or you could line wrap it, but as you say, that probably makes things worse.) 3. Just delete commented out code like that. If anyone needs it, it is in git. If there is a good reason to keep it, then add words to the comment, explaining why, which should make codechecker stop complaining. Or, if the worst comes to worst, leave that as is.
          Hide
          jmvedrine Jean-Michel Vedrine added a comment -

          Hello Tim,
          Rebased this. Unfortunately an upgrade of the codechecker revealed many underscores in variables names that were not previously reported.
          So I fixed hem (currently in a separate commit will squash to one commit after your review)
          To save you the hassle of runnning the codechecker here is the output:
          Total: 3 error(s) and 2 warning(s)
          question\format\gift\format.php
          ················$expout·.=·"\$CATEGORY:·$question->category\n";
          642: Variable "CATEGORY" must be all lower-case
          question\format\xml\tests\xmlformat_test.php
          ······<text><![CDATA[Complete·this·opening·line·of·verse:·"The·

          {1:SHORTANSWER:Dog#Wrong,·silly!~=Owl#Well·done!~*#Wrong·answer}

          ·and·the·

          {1:MULTICHOICE:Bow-wow#You·seem·to·have·a·dog·obsessions!~Wiggly·worm#Now·you·are·just·being·ridiculous!~=Pussy-cat#Well·done!}

          ·went·to·sea".]]></text>
          1289: Line exceeds maximum limit of 180 characters; contains 287 characters
          ······<text><![CDATA[Complete·this·opening·line·of·verse:·"The·

          {1:SHORTANSWER:Dog#Wrong,·silly!~=Owl#Well·done!~*#Wrong·answer}

          ·and·the·

          {1:MULTICHOICE:Bow-wow#You·seem·to·have·a·dog·obsessions!~Wiggly·worm#Now·you·are·just·being·ridiculous!~=Pussy-cat#Well·done!}

          ·went·to·sea".]]></text>
          1397: Line exceeds maximum limit of 180 characters; contains 287 characters
          ······<text><![CDATA[General·feedback:·It\'s·from·"The·Owl·and·the·Pussy-cat"·by·Lear:·"The·owl·and·the·pussycat·went·to·sea".]]></text>
          1292: Line exceeds 132 characters; contains 136 characters
          ······<text><![CDATA[General·feedback:·It\'s·from·"The·Owl·and·the·Pussy-cat"·by·Lear:·"The·owl·and·the·pussycat·went·to·sea]]></text>
          1400: Line exceeds 132 characters; contains 134 characters

          As you see only the "error" wrongly reported for the gift format $CATEGORY, and 4 too long lines in xml format's tests.

          Show
          jmvedrine Jean-Michel Vedrine added a comment - Hello Tim, Rebased this. Unfortunately an upgrade of the codechecker revealed many underscores in variables names that were not previously reported. So I fixed hem (currently in a separate commit will squash to one commit after your review) To save you the hassle of runnning the codechecker here is the output: Total: 3 error(s) and 2 warning(s) question\format\gift\format.php ················$expout·.=·"\$CATEGORY:·$question->category\n"; 642: Variable "CATEGORY" must be all lower-case question\format\xml\tests\xmlformat_test.php ······<text><![CDATA[Complete·this·opening·line·of·verse:·"The· {1:SHORTANSWER:Dog#Wrong,·silly!~=Owl#Well·done!~*#Wrong·answer} ·and·the· {1:MULTICHOICE:Bow-wow#You·seem·to·have·a·dog·obsessions!~Wiggly·worm#Now·you·are·just·being·ridiculous!~=Pussy-cat#Well·done!} ·went·to·sea".]]></text> 1289: Line exceeds maximum limit of 180 characters; contains 287 characters ······<text><![CDATA[Complete·this·opening·line·of·verse:·"The· {1:SHORTANSWER:Dog#Wrong,·silly!~=Owl#Well·done!~*#Wrong·answer} ·and·the· {1:MULTICHOICE:Bow-wow#You·seem·to·have·a·dog·obsessions!~Wiggly·worm#Now·you·are·just·being·ridiculous!~=Pussy-cat#Well·done!} ·went·to·sea".]]></text> 1397: Line exceeds maximum limit of 180 characters; contains 287 characters ······<text><![CDATA [General·feedback:·It\'s·from·"The·Owl·and·the·Pussy-cat"·by·Lear:·"The·owl·and·the·pussycat·went·to·sea".] ]></text> 1292: Line exceeds 132 characters; contains 136 characters ······<text><![CDATA [General·feedback:·It\'s·from·"The·Owl·and·the·Pussy-cat"·by·Lear:·"The·owl·and·the·pussycat·went·to·sea] ]></text> 1400: Line exceeds 132 characters; contains 134 characters As you see only the "error" wrongly reported for the gift format $CATEGORY, and 4 too long lines in xml format's tests.
          Hide
          jmvedrine Jean-Michel Vedrine added a comment -

          Asking for peer review

          Show
          jmvedrine Jean-Michel Vedrine added a comment - Asking for peer review
          Hide
          timhunt Tim Hunt added a comment -

          I think you should report the $CATEGORY thing as a bug in codechecker. (There is a component for it in the CONTRIB project.)

          The patch looks basically OK, and I trust you, but I cannot claim I have reviewed every line of the patch in detail.

          Can you write some testing instructions, if those look OK, and you rebase, I think we can submit this for integration.

          Show
          timhunt Tim Hunt added a comment - I think you should report the $CATEGORY thing as a bug in codechecker. (There is a component for it in the CONTRIB project.) The patch looks basically OK, and I trust you, but I cannot claim I have reviewed every line of the patch in detail. Can you write some testing instructions, if those look OK, and you rebase, I think we can submit this for integration.
          Hide
          jmvedrine Jean-Michel Vedrine added a comment -

          Hi Tim,
          I already reported the $CATEGORY problem in the tracker as soon as I encountered it, see CONTRIB-4286 "Codechecker don't correctly identify escaped $ in strings".
          This kind of patch makes me nervous, because it changes many lines in many files, so it's difficult to verify nothing is broken. I have read the diff many times, and greped the code before changing any variables name to ensure the new name was not used, but I must confess I can't be 100% sure there is not a stupid mistake or typo

          Show
          jmvedrine Jean-Michel Vedrine added a comment - Hi Tim, I already reported the $CATEGORY problem in the tracker as soon as I encountered it, see CONTRIB-4286 "Codechecker don't correctly identify escaped $ in strings". This kind of patch makes me nervous, because it changes many lines in many files, so it's difficult to verify nothing is broken. I have read the diff many times, and greped the code before changing any variables name to ensure the new name was not used, but I must confess I can't be 100% sure there is not a stupid mistake or typo
          Hide
          timhunt Tim Hunt added a comment -

          Yes, but this is Moodle 2.6-only, and so if you have broken anything, there is time to fix it before the 2.6 release. It is good that you are worried

          Show
          timhunt Tim Hunt added a comment - Yes, but this is Moodle 2.6-only, and so if you have broken anything, there is time to fix it before the 2.6 release. It is good that you are worried
          Hide
          jmvedrine Jean-Michel Vedrine added a comment -

          Hello Tim,
          Can you have a look to see if this is ready for integration ?

          Show
          jmvedrine Jean-Michel Vedrine added a comment - Hello Tim, Can you have a look to see if this is ready for integration ?
          Hide
          timhunt Tim Hunt added a comment -

          Yes, will be good to get this integrated. Thanks.

          Show
          timhunt Tim Hunt added a comment - Yes, will be good to get this integrated. Thanks.
          Hide
          poltawski Dan Poltawski added a comment -

          Integrated to master, thanks!

          Show
          poltawski Dan Poltawski added a comment - Integrated to master, thanks!
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Thanks guys - tests passed.

          Show
          samhemelryk Sam Hemelryk added a comment - Thanks guys - tests passed.
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Against all probability we've achieved normality. You changes didn't break the tests I pretended to run and are now immortalised upstream. Good for you!

          "It was a programming technique that had been reverse-engineered from the sort of psychotic mental blocks that otherwise perfectly normal people had been observed invariably to develop when elected to high political office."
          Adams, D (1992) Mostly Harmless. London: William Heinemann.

          Show
          samhemelryk Sam Hemelryk added a comment - Against all probability we've achieved normality. You changes didn't break the tests I pretended to run and are now immortalised upstream. Good for you! "It was a programming technique that had been reverse-engineered from the sort of psychotic mental blocks that otherwise perfectly normal people had been observed invariably to develop when elected to high political office." Adams, D (1992) Mostly Harmless. London: William Heinemann.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                18/Nov/13