Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Won't Fix
    • Affects Version/s: 2.4.1
    • Fix Version/s: None
    • Component/s: Local: Code checker
    • Labels:
      None
    • Rank:
      47710

      Description

      Sometimes you wish to type:

      // The following is really awesome:
      $a = $a + 1;
      

      However the code checker reports "Inline comments must end in full-stops, exclamation marks, or question marks".

      Therefore it is desirable for colons (':') to be accepted.

      Edited by Eloy to clarify it's about to allow colons before code (i.e. at the end of the comment). They are valid everywhere else within the comment already.

        Issue Links

          Activity

          Hide
          Eloy Lafuente (stronk7) added a comment -

          Sorry, after thinking, as commented @ HQ chat, this gets my -1:

          • Replace colons by dots, anybody unable to understand the result?
          • Comments are self-contained, hence only final symbols are allowed.
          • Opening the door to non-final symbols implies accepting many more symbols.
          • Arguing the readability of mixed comments and code has no sense. Because if you accept that, you should also provide some coding rule to express that the next comments come together with the code => doh?
          • Reducing the proposal to the absurd, it could be considered that all the comments are always the preamble to the following code lines, so we should, always use colons in all comments => crazy.
          • The codechecker returns colons as warnings, not as errors, who cares about warnings.

          Ciao

          PS: I don't care, really!

          Show
          Eloy Lafuente (stronk7) added a comment - Sorry, after thinking, as commented @ HQ chat, this gets my -1: Replace colons by dots, anybody unable to understand the result? Comments are self-contained, hence only final symbols are allowed. Opening the door to non-final symbols implies accepting many more symbols. Arguing the readability of mixed comments and code has no sense. Because if you accept that, you should also provide some coding rule to express that the next comments come together with the code => doh? Reducing the proposal to the absurd, it could be considered that all the comments are always the preamble to the following code lines, so we should, always use colons in all comments => crazy. The codechecker returns colons as warnings, not as errors, who cares about warnings. Ciao PS: I don't care, really!
          Hide
          Gareth J Barnard added a comment -

          Sorry, did not intend to light the blue touch paper, was just a request in the plugin's database.

          I do read warnings as they can help to prevent future issues.

          I'm not proposing to use colons everywhere. Just to use them when I consider appropriate without being nagged at.

          Up to you if you implement this. Have now figured out how to use my own branch of the code.

          Show
          Gareth J Barnard added a comment - Sorry, did not intend to light the blue touch paper, was just a request in the plugin's database. I do read warnings as they can help to prevent future issues. I'm not proposing to use colons everywhere. Just to use them when I consider appropriate without being nagged at. Up to you if you implement this. Have now figured out how to use my own branch of the code.
          Hide
          Gareth J Barnard added a comment -

          P.S. I wish I'd never asked!

          Show
          Gareth J Barnard added a comment - P.S. I wish I'd never asked!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          No probs, Gareth!

          Every idea is welcome, 100%. Different opinions aren't nothing but, simply, different opinions.

          FYI, I've raised this to the integration team for getting the final decision.

          Thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - No probs, Gareth! Every idea is welcome, 100%. Different opinions aren't nothing but, simply, different opinions. FYI, I've raised this to the integration team for getting the final decision. Thanks!
          Hide
          Martin Dougiamas added a comment -

          I would like to note that Eloy, in his first reply, actually ended his first sentence with a colon.

          My +1 for allowing colons because it's a really common sentence structure for programmers who like lists.

          Show
          Martin Dougiamas added a comment - I would like to note that Eloy, in his first reply, actually ended his first sentence with a colon. My +1 for allowing colons because it's a really common sentence structure for programmers who like lists.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Martin, you are missing the point. It's about to allow colons at the end of the comment, not in the middle (it is perfectly valid there).

          // Would you accept this ? (as a valid php comment):

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Martin, you are missing the point. It's about to allow colons at the end of the comment, not in the middle (it is perfectly valid there). // Would you accept this ? (as a valid php comment):
          Hide
          Martin Dougiamas added a comment -

          Ah in that case I retract my +1, but this has got to be the smallest paintshed ever.

          What about a comment like this, will it give an error?

          // Your mileage may vary

          If so that's a bit ridiculous to be warning about. It's a comment.

          Show
          Martin Dougiamas added a comment - Ah in that case I retract my +1, but this has got to be the smallest paintshed ever. What about a comment like this, will it give an error? // Your mileage may vary If so that's a bit ridiculous to be warning about. It's a comment .
          Hide
          Mary Evans added a comment - - edited

          In English Grammar we always used the abbreviation :- which means as follows:-
          With the advent of the Internet and the many 'text' emoticons that can be coded in emails and text messages, this useful abbreviation has seen a demise in it's use. Probably because people think it looks rude.

          Show
          Mary Evans added a comment - - edited In English Grammar we always used the abbreviation :- which means as follows:- With the advent of the Internet and the many 'text' emoticons that can be coded in emails and text messages, this useful abbreviation has seen a demise in it's use. Probably because people think it looks rude.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Heh, and you tell me it's a paintshed. I did not open the box, lol.

          About the "correct" punctuation (dots and friends) being detected and warned, it was discussed and agreed a bunch of months ago, and I'm not going to reopen that discussion. They are recommended (hence, they are warnings, not errors), and that is it.

          But as far as this request has been done via proper issue, it needs to be voted and implemented or closed. I'm awaiting integrators to give their ±1 and done.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Heh, and you tell me it's a paintshed. I did not open the box, lol. About the "correct" punctuation (dots and friends) being detected and warned, it was discussed and agreed a bunch of months ago, and I'm not going to reopen that discussion. They are recommended (hence, they are warnings, not errors), and that is it. But as far as this request has been done via proper issue, it needs to be voted and implemented or closed. I'm awaiting integrators to give their ±1 and done. Ciao
          Hide
          Mary Evans added a comment -

          +1 from me:

          Show
          Mary Evans added a comment - +1 from me:
          Hide
          Gareth J Barnard added a comment -

          I never realised that the box I opened had the name of 'Pandora' on it.

          Show
          Gareth J Barnard added a comment - I never realised that the box I opened had the name of 'Pandora' on it.
          Hide
          Dan Poltawski added a comment -

          +1 for:

          // This user is really cool:
          $dan = $poltawski[0];
          
          Show
          Dan Poltawski added a comment - +1 for: // This user is really cool: $dan = $poltawski[0];
          Hide
          Aparup Banerjee added a comment -

          -1 for : at the end of a comment where the next line is code.
          if the code is moved about there is structure to group it up. there is no structure to contain the comment together with the code it is referencing with it's ':'.

          Show
          Aparup Banerjee added a comment - -1 for : at the end of a comment where the next line is code. if the code is moved about there is structure to group it up. there is no structure to contain the comment together with the code it is referencing with it's ':'.
          Hide
          Damyon Wiese added a comment -

          +1 for

          Because I am +1 for loosening (a little) the strictness of the non-code warnings in the code checker in general. I think too many warnings are thrown by existing code which stops people writing bug fixes or improvements from using the code checker usefully.

          Show
          Damyon Wiese added a comment - +1 for Because I am +1 for loosening (a little) the strictness of the non-code warnings in the code checker in general. I think too many warnings are thrown by existing code which stops people writing bug fixes or improvements from using the code checker usefully.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Offtopic, about Damyon comment: Well, main problem is that our codebase is plagued of codechecker errors and warnings, and the checker does it job for complete files, not for patches (and the results are always depressing no matter the patch is 100% correct or no).
          I hope the prechecker (that has the ability to restrict output to the patched lines) will help about that.

          Uhm, perhaps we could try to build that in the codechecker, say:

          0) if the site is git-based
          1) add one new setting: "branch base"
          2) extract the lines from the diff
          3) filter the results out
          4) yum!

          Edited: This idea has been moved to CONTRIB-4163, for reference.

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Offtopic, about Damyon comment: Well, main problem is that our codebase is plagued of codechecker errors and warnings, and the checker does it job for complete files, not for patches (and the results are always depressing no matter the patch is 100% correct or no). I hope the prechecker (that has the ability to restrict output to the patched lines) will help about that. Uhm, perhaps we could try to build that in the codechecker, say: 0) if the site is git-based 1) add one new setting: "branch base" 2) extract the lines from the diff 3) filter the results out 4) yum! Edited: This idea has been moved to CONTRIB-4163 , for reference.
          Hide
          Sam Hemelryk added a comment -

          lol -1 from me.

          What a fantastic issue this has turned out to be, such a small request, such a small change, and such a divided vote!

          My subjective rational. It's just a warning and I would not be pulling anyone up on it during integration.
          I like to think of the code checker as a guide, errors are problems, warning are recommendations.
          If nothing else it shows the personality of the author. But I don't want a rule to say its allowed, as Eloy notes colon's are just one of many characters that people use to finish sentences and I'd rather people say "I'm going to ignore that warning" rather than having to look up a table of acceptable characters to use at the end of a comment.

          By the way this whole thing is the smallest issue but it has brought a smile to my face.

          Many thanks indeed!
          Sam

          Show
          Sam Hemelryk added a comment - lol -1 from me. What a fantastic issue this has turned out to be, such a small request, such a small change, and such a divided vote! My subjective rational. It's just a warning and I would not be pulling anyone up on it during integration. I like to think of the code checker as a guide, errors are problems, warning are recommendations. If nothing else it shows the personality of the author. But I don't want a rule to say its allowed, as Eloy notes colon's are just one of many characters that people use to finish sentences and I'd rather people say "I'm going to ignore that warning" rather than having to look up a table of acceptable characters to use at the end of a comment. By the way this whole thing is the smallest issue but it has brought a smile to my face. Many thanks indeed! Sam
          Hide
          Gareth J Barnard added a comment -

          I never realised that the rocket attached to the blue touch paper I lit was made by NASA. All I wanted was a very small change to reduce the amount of output I had to process and segment to get to the really important messages. Without the warnings from a computer attempting to teach me my own language information overload is reduced and 'true' recommendations have more prominence.

          I now ask the value of checking comments at all?

          And as for showing the personality of the author on a subject of 'colons' perhaps that leads to something entirely different from the intended

          So please can we have messages about the important and no messages about the trivial

          Show
          Gareth J Barnard added a comment - I never realised that the rocket attached to the blue touch paper I lit was made by NASA. All I wanted was a very small change to reduce the amount of output I had to process and segment to get to the really important messages. Without the warnings from a computer attempting to teach me my own language information overload is reduced and 'true' recommendations have more prominence. I now ask the value of checking comments at all? And as for showing the personality of the author on a subject of 'colons' perhaps that leads to something entirely different from the intended So please can we have messages about the important and no messages about the trivial
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Oki, so this, finally, became rejected, thanks all. Closing as won't fix.

          Gareth, LOL, to make the code-checker able to only inform about errors is a possibility that can be implemented easily (I think). In fact the CLI (phpcs) supports it already (I use the CLI often than then UI):

          phpcs --standard=local/codechecker/moodle -n mod/label
          (-n switches off warnings)
          

          So I'd suggest you to create an issue for that, TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Oki, so this, finally, became rejected, thanks all. Closing as won't fix. Gareth, LOL, to make the code-checker able to only inform about errors is a possibility that can be implemented easily (I think). In fact the CLI (phpcs) supports it already (I use the CLI often than then UI): phpcs --standard=local/codechecker/moodle -n mod/label (-n switches off warnings) So I'd suggest you to create an issue for that, TIA and ciao
          Hide
          Gareth J Barnard added a comment -

          Dear Eloy,

          Ta

          I'll code check my code against my fork of the project in future .

          Time for blue touch paper No.2 after getting to it on my squeaky wheeled bicycle . But might leave it a while for far more important bugs to get a share of the light and be sorted.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Eloy, Ta I'll code check my code against my fork of the project in future . Time for blue touch paper No.2 after getting to it on my squeaky wheeled bicycle . But might leave it a while for far more important bugs to get a share of the light and be sorted. Cheers, Gareth
          Hide
          Eloy Lafuente (stronk7) added a comment -

          LOL, Gareth, just note this ended being soooo funny that I could not resist to add a reference to the issue in the latest integration notes:

          https://moodle.org/mod/forum/discuss.php?d=223423

          Cheers & ciao

          Show
          Eloy Lafuente (stronk7) added a comment - LOL, Gareth, just note this ended being soooo funny that I could not resist to add a reference to the issue in the latest integration notes: https://moodle.org/mod/forum/discuss.php?d=223423 Cheers & ciao
          Hide
          Mary Evans added a comment -
          Show
          Mary Evans added a comment - Just for phun!
          Hide
          Gareth J Barnard added a comment -

          No worries all

          I just wish the code checker warnings were positive improvement ones rather than pedantic ones.

          Show
          Gareth J Barnard added a comment - No worries all I just wish the code checker warnings were positive improvement ones rather than pedantic ones.

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development