Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9.3
    • Fix Version/s: 1.9.4
    • Component/s: Questions
    • Labels:
      None
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE
    • Rank:
      35460

      Description

      Convert shortanswer qtype to use extra_question_fields() mechanism for option handling, patch will be mine.

      1. shortanswer_extrafields.patch
        6 kB
        Oleg Sychev
      2. shortanswer_extrafields.patch
        8 kB
        Oleg Sychev
      3. shortanswer_extrafields.patch
        6 kB
        Oleg Sychev

        Issue Links

          Activity

          Hide
          Oleg Sychev added a comment -

          I tried to do this, and test the result. Easy thing, with one exception - extra_question_fields() awaits "questionid" field, while in shortanswer (and other core questiontypes) have it as "question". Should we bother with bumping up version and renaming field, or is it better to correct implementation of extra_question_fields() mechanism to use "question" as field name?

          The patch will be available shortly after you answered to the question above. About 20 strings could be deleted from shortanswer there.

          Show
          Oleg Sychev added a comment - I tried to do this, and test the result. Easy thing, with one exception - extra_question_fields() awaits "questionid" field, while in shortanswer (and other core questiontypes) have it as "question". Should we bother with bumping up version and renaming field, or is it better to correct implementation of extra_question_fields() mechanism to use "question" as field name? The patch will be available shortly after you answered to the question above. About 20 strings could be deleted from shortanswer there.
          Hide
          Oleg Sychev added a comment -

          Oh, it seems to many things depends on "question" field name. Better correct extra_question_fields() mechanism IMHO. "questionid" is definitely a better name, but changing this fields can affects too many things and may results in error.

          Show
          Oleg Sychev added a comment - Oh, it seems to many things depends on "question" field name. Better correct extra_question_fields() mechanism IMHO. "questionid" is definitely a better name, but changing this fields can affects too many things and may results in error.
          Hide
          Tim Hunt added a comment -

          The reason for the question/questionid discrepancy is that a long time ago, but after the quiz was first written, the coding guidelines for how to name database columns changed.

          The best solution is to rename the column, and fix everything that depends on it, but that is quite a big job.

          It would probably be OK to add a optional $usedeprecatedcolname = false argument to extra_question_fields to tell it use the old column name.

          Show
          Tim Hunt added a comment - The reason for the question/questionid discrepancy is that a long time ago, but after the quiz was first written, the coding guidelines for how to name database columns changed. The best solution is to rename the column, and fix everything that depends on it, but that is quite a big job. It would probably be OK to add a optional $usedeprecatedcolname = false argument to extra_question_fields to tell it use the old column name.
          Hide
          Oleg Sychev added a comment -

          Oops, use deprecated field name should not be an argument of extra_question_fileds, it's a part of returned value. I think that better way to keep compatibility is to use additional function with default implementation. A referenced argument can be used, but it hard to make it optional; and the return value is already complex thing to add something.

          See attached patch.

          Show
          Oleg Sychev added a comment - Oops, use deprecated field name should not be an argument of extra_question_fileds, it's a part of returned value. I think that better way to keep compatibility is to use additional function with default implementation. A referenced argument can be used, but it hard to make it optional; and the return value is already complex thing to add something. See attached patch.
          Hide
          Oleg Sychev added a comment -

          It seems that "answers" field in "question_shortanswer" table (not $question->options->answers, which is filled from "question_answers" table directly) is obsolete (or I'm missing something). I found no code using it, except it filling in save_question_options, and code for backuping and restoring it. Should we delete code with handling it or not?

          Show
          Oleg Sychev added a comment - It seems that "answers" field in "question_shortanswer" table (not $question->options->answers, which is filled from "question_answers" table directly) is obsolete (or I'm missing something). I found no code using it, except it filling in save_question_options, and code for backuping and restoring it. Should we delete code with handling it or not?
          Hide
          Tim Hunt added a comment -

          Yes, it is obsolete, and should be cleaned up one day, but it does not do any hard having it there, so cleaning it up has never been a priority.

          Show
          Tim Hunt added a comment - Yes, it is obsolete, and should be cleaned up one day, but it does not do any hard having it there, so cleaning it up has never been a priority.
          Hide
          Oleg Sychev added a comment -

          It would be great to clean it up working on MDL-17229 as it makes MDL-17800 and subsequent changes to shortanswer much easier.
          (btw, I think there is a bug in backuping similar field in numerical question (backup/restore accept it to be a single number, while user interface not) and no one still notice it, so the field must be really unused).

          Should I delete handling "answers" field from save_question_options in this patch? And what you think about patch there? Knowing that you like to clean up things by the way I may also replace unnecesary double quotes in save_question_options by single quotes, if you wish.

          Show
          Oleg Sychev added a comment - It would be great to clean it up working on MDL-17229 as it makes MDL-17800 and subsequent changes to shortanswer much easier. (btw, I think there is a bug in backuping similar field in numerical question (backup/restore accept it to be a single number, while user interface not) and no one still notice it, so the field must be really unused). Should I delete handling "answers" field from save_question_options in this patch? And what you think about patch there? Knowing that you like to clean up things by the way I may also replace unnecesary double quotes in save_question_options by single quotes, if you wish.
          Hide
          Tim Hunt added a comment -

          Would be nice to get rid of answers. The only thing is ensuring that it gets enough testing. It probably has to be Moodle 2.0 only.

          Show
          Tim Hunt added a comment - Would be nice to get rid of answers. The only thing is ensuring that it gets enough testing. It probably has to be Moodle 2.0 only.
          Hide
          Oleg Sychev added a comment -

          Ok, cleaning "answers" can wait, I'll create a separate issue on it. Could you apply one of the patches on this task, so I can work on another issues with questiontype.php? The second patch features also double to single quotes change.

          Show
          Oleg Sychev added a comment - Ok, cleaning "answers" can wait, I'll create a separate issue on it. Could you apply one of the patches on this task, so I can work on another issues with questiontype.php? The second patch features also double to single quotes change.
          Hide
          Tim Hunt added a comment -

          Oh yes, I never replied to that bit.

          As a general rule, patches that unnecessarily mess around with whitespace, quoting style, etc. are a bad thing for two reasons:
          1. it makes it harder to merge changes from one branch to another (e.g. 1.9 stable to HEAD) if there are irrelevant differences between the files; and
          2. it obscures CVS history, and CVS is very helpful for understanding code.

          On the other hand, nice clean code is important too. So the best thing to do is:
          A. Be very careful to write good code that follows the coding guidelines in the first place.
          B. In any lines of code that you do have to change for your patch, then you can fix all the coding style problems in those lines.

          This is basically the same philosopy as the internet "Be strict when sending and tolerant when receiving." (3.9 in http://www.ietf.org/rfc/rfc1958.txt).

          So, anyway, I'll review any commit (if OK) the patch without the extra changes.

          Show
          Tim Hunt added a comment - Oh yes, I never replied to that bit. As a general rule, patches that unnecessarily mess around with whitespace, quoting style, etc. are a bad thing for two reasons: 1. it makes it harder to merge changes from one branch to another (e.g. 1.9 stable to HEAD) if there are irrelevant differences between the files; and 2. it obscures CVS history, and CVS is very helpful for understanding code. On the other hand, nice clean code is important too. So the best thing to do is: A. Be very careful to write good code that follows the coding guidelines in the first place. B. In any lines of code that you do have to change for your patch, then you can fix all the coding style problems in those lines. This is basically the same philosopy as the internet "Be strict when sending and tolerant when receiving." (3.9 in http://www.ietf.org/rfc/rfc1958.txt ). So, anyway, I'll review any commit (if OK) the patch without the extra changes.
          Hide
          Tim Hunt added a comment -

          Some very picky and pedantic code review comments:

          1. I like a single blank line between functions.

          2. $qid_col_name= $this->deprecated_qid_col_name() ? 'question':'questionid';

          • variable names should not contain underscores (http://docs.moodle.org/en/Development:Coding#Coding_style) (Oh dear, some of my code in that function is breaking that rule.)
          • Should have a space before the = sign, or, at least the same number of spaces each side.
          • (The is personal preference, not in the coding guidelines) I tend to avoid ? : - sure I know what it means, and you know what it means, but it is still less readable than a proper if statement.

          3. In
          $question->answers=implode(",",$answers);
          $parentresult=parent:: save_question_options($question);
          if($parentresult!==null) {//parent function returns null if all is OK
          return $parentresult;
          My preferred whitespace style would be
          $question->answers = implode(",", $answers);
          $parentresult = parent::save_question_options($question);
          if ($parentresult !== null) { // Parent function returns null if all is OK
          return $parentresult;
          Again, some of this is personal preference, rather than official coding guidelines, but look at the example code in http://docs.moodle.org/en/Development:Coding, and you will see it is closer to my style.

          Sorry to be so picky. But what you are doing is very good, so one day I would like to trust you with CVS commit access, but for that, you need to get things right first time, which is why I am being very harsh on you now. I hope that is OK.

          A more significant comment: It would simplify the code to change the function

          function deprecated_qid_col_name()

          { return false; }

          to be

          function questionid_column_name()

          { return 'questionid'; }

          That would remove the need for some of the conditional statements.

          By the way, to answer another of your questions. The best way to handle multiple separate sets of patches is to use a better version control system like git. http://git-scm.com/

          Failing that, you can always save one patch, then undo your changes while you work on the other thing. You can always re-apply the first patch when you want to work on that again. This only works when the two changes are unrelated.

          If you can generate a revised patch, I will try to apply it ASAP. Thanks.

          Show
          Tim Hunt added a comment - Some very picky and pedantic code review comments: 1. I like a single blank line between functions. 2. $qid_col_name= $this->deprecated_qid_col_name() ? 'question':'questionid'; variable names should not contain underscores ( http://docs.moodle.org/en/Development:Coding#Coding_style ) (Oh dear, some of my code in that function is breaking that rule.) Should have a space before the = sign, or, at least the same number of spaces each side. (The is personal preference, not in the coding guidelines) I tend to avoid ? : - sure I know what it means, and you know what it means, but it is still less readable than a proper if statement. 3. In $question->answers=implode(",",$answers); $parentresult=parent:: save_question_options($question); if($parentresult!==null) {//parent function returns null if all is OK return $parentresult; My preferred whitespace style would be $question->answers = implode(",", $answers); $parentresult = parent::save_question_options($question); if ($parentresult !== null) { // Parent function returns null if all is OK return $parentresult; Again, some of this is personal preference, rather than official coding guidelines, but look at the example code in http://docs.moodle.org/en/Development:Coding , and you will see it is closer to my style. Sorry to be so picky. But what you are doing is very good, so one day I would like to trust you with CVS commit access, but for that, you need to get things right first time, which is why I am being very harsh on you now. I hope that is OK. A more significant comment: It would simplify the code to change the function function deprecated_qid_col_name() { return false; } to be function questionid_column_name() { return 'questionid'; } That would remove the need for some of the conditional statements. By the way, to answer another of your questions. The best way to handle multiple separate sets of patches is to use a better version control system like git. http://git-scm.com/ Failing that, you can always save one patch, then undo your changes while you work on the other thing. You can always re-apply the first patch when you want to work on that again. This only works when the two changes are unrelated. If you can generate a revised patch, I will try to apply it ASAP. Thanks.
          Hide
          Oleg Sychev added a comment -

          Revised patch attached. I give it more testing, to be sure not break anything.

          "B. In any lines of code that you do have to change for your patch, then you can fix all the coding style problems in those lines. " - I was unsure with moved line $question->answers = implode(",", $answers); , but since diff thinks this line to be deleted and inserted even when unchanged (see the very first patch), I suggest I still must change quotes in it.

          " tend to avoid ? : - sure I know what it means, and you know what it means, but it is still less readable than a proper if statement." - I'm not very sure about readability, as in complex functions one line can be more readable than five. I'm certainly against using ? : with more complex expressions; and there is no need to use them in the patch now anyway.

          "Sorry to be so picky. But what you are doing is very good, so one day I would like to trust you with CVS commit access, but for that, you need to get things right first time, which is why I am being very harsh on you now. I hope that is OK. " - it's no problem at all, don't worry (and you are very courteous to me even when "harsh", thanks). I can follow any clear defined rules (that's you project after all). I'm feeling much more uneasy with undefined rules, that I must learn by trial and error, or with delays for vague time.

          Show
          Oleg Sychev added a comment - Revised patch attached. I give it more testing, to be sure not break anything. "B. In any lines of code that you do have to change for your patch, then you can fix all the coding style problems in those lines. " - I was unsure with moved line $question->answers = implode(",", $answers); , but since diff thinks this line to be deleted and inserted even when unchanged (see the very first patch), I suggest I still must change quotes in it. " tend to avoid ? : - sure I know what it means, and you know what it means, but it is still less readable than a proper if statement." - I'm not very sure about readability, as in complex functions one line can be more readable than five. I'm certainly against using ? : with more complex expressions; and there is no need to use them in the patch now anyway. "Sorry to be so picky. But what you are doing is very good, so one day I would like to trust you with CVS commit access, but for that, you need to get things right first time, which is why I am being very harsh on you now. I hope that is OK. " - it's no problem at all, don't worry (and you are very courteous to me even when "harsh", thanks). I can follow any clear defined rules (that's you project after all). I'm feeling much more uneasy with undefined rules, that I must learn by trial and error, or with delays for vague time.
          Hide
          Oleg Sychev added a comment -

          ... and last not least - the most appreciated thing is that you describe you reasons when criticise something. This is actualy useful (and rare) thing, many thanks for doing that.

          Show
          Oleg Sychev added a comment - ... and last not least - the most appreciated thing is that you describe you reasons when criticise something. This is actualy useful (and rare) thing, many thanks for doing that.
          Hide
          Tim Hunt added a comment -

          Checked in now. Nice work Oleg. Thank you.

          Just one other picky comment, there should be no whitespace at the end of lines, and you had a couple of blank lines with spaces in. (I use Eclipse which has a 'make whitespace visible' option, which is very helpful.)

          Show
          Tim Hunt added a comment - Checked in now. Nice work Oleg. Thank you. Just one other picky comment, there should be no whitespace at the end of lines, and you had a couple of blank lines with spaces in. (I use Eclipse which has a 'make whitespace visible' option, which is very helpful.)

            People

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

              Dates

              • Created:
                Updated:
                Resolved: