Uploaded image for project: 'Request For Comments'
  1. Request For Comments
  2. RFC-205

Disallow "if False:" for "block comments" in python

    XMLWordPrintable

    Details

    • Type: RFC
    • Status: Implemented
    • Resolution: Done
    • Component/s: DM
    • Labels:
      None

      Description

      There seems to be an unwritten convention in LSST to use if False: to "comment out" blocks of python code. I had a community post about why this is bad practice in December 2015, but didn't get much discussion at the time. Since it's come up again in some reviews, I'd like to address it head on here. (note: The same arguments apply to if True:.)

      The argument I've been given about this is that it's a "pythonification of a C++ standard," referencing this C++ standards page. Using #if 0 (or equivalent) in C++ makes some sense in light of the C pre-processor. In python, if debugFlag is True: is useful for code one wants to turn on/off with a high level flag. However, if False: it is a very poor choice for python for the following reasons:

      1. the code is still run by the interpreter.
      2. it doesn't look like a comment: editors show it as valid code.
      3. when reading the code, you have to visually parse the if block to realize it is unused.
      4. the code is now at a different indentation level, potentially causing problems if the code is restored, and potentially messing up auto-indent of subsequent else: blocks, if there are if statements inside the if False.
      5. It's no easier than commenting with #: editors can trivially prepend {{# }} to a region of code with a single keystroke.
      6. it's not pythonic: it is rarely used by python developers who were not first C/C++ developers, and it is not a generally accepted community "block comment" method (pre-pending # or triple-quotes are the standard).

      Our python coding standards say nothing about disallowing commented-out code blocks in production code. I generally agree that that is good practice-I would suggest we discourage but not disallow it-but using if False is much worse than commenting out the code blocks with #, for the above reasons.

      Our python coding standards say nothing about any of this. I believe our standards should explicitly disallow if True:/if False: in all cases, and discourage commented-out code, with exceptions allowed for if the code includes a comment with a clear timeline for when that block would be either removed or integrated back (e.g. Jira ticket number).

        Attachments

          Issue Links

            Activity

            Hide
            ktl Kian-Tat Lim added a comment -

            John Parejko: Can I get a concrete, crisp phrasing of the resulting change to the coding standards that you would like to implement following this discussion?

            Show
            ktl Kian-Tat Lim added a comment - John Parejko : Can I get a concrete, crisp phrasing of the resulting change to the coding standards that you would like to implement following this discussion?
            Hide
            Parejkoj John Parejko added a comment -

            Code must not be placed inside `if False:` or `if True:` blocks, nor left commented out. If one has debugging code or if one is undecided about which particular implementation to use, such code must be placed inside a "named" `if` statement. Such blocks may have a comment describing the conditions under which said code can be removed (e.g. completion of a ticket, a particular date). For example, for code that will likely be removed in the future, once testing is completed:

            # Delete old_thing() and this "if" once we have all the unittests in place (DM-123456).
            use_old_method = False
            if use_old_method:
                old_thing()
            else:
                new_thing()
            

            Such debugging flags should usually be lifted up into the method's keyword arguments to allow users to decide which branch to run. For example:

            def foo(x, debug_plots=False):
                do_thing()
                if debug_plots:
                    plot_thing()
            

            or, using lsstDebug, which can be controlled as part of a commandline task:

            import lsstDebug
            def foo(x):
                do_thing()
                if lsstDebug.Info(__name__).debug_plots:
                    plot_thing()
            

            Show
            Parejkoj John Parejko added a comment - Code must not be placed inside `if False:` or `if True:` blocks, nor left commented out. If one has debugging code or if one is undecided about which particular implementation to use, such code must be placed inside a "named" `if` statement. Such blocks may have a comment describing the conditions under which said code can be removed (e.g. completion of a ticket, a particular date). For example, for code that will likely be removed in the future, once testing is completed: # Delete old_thing() and this "if" once we have all the unittests in place (DM-123456). use_old_method = False if use_old_method: old_thing() else: new_thing() Such debugging flags should usually be lifted up into the method's keyword arguments to allow users to decide which branch to run. For example: def foo(x, debug_plots=False): do_thing() if debug_plots: plot_thing() or, using lsstDebug , which can be controlled as part of a commandline task: import lsstDebug def foo(x): do_thing() if lsstDebug.Info(__name__).debug_plots: plot_thing()
            Hide
            rowen Russell Owen added a comment -

            This looks fine to me.

            Show
            rowen Russell Owen added a comment - This looks fine to me.
            Hide
            krughoff Simon Krughoff added a comment -

            I like the wording too.

            Show
            krughoff Simon Krughoff added a comment - I like the wording too.
            Hide
            Parejkoj John Parejko added a comment -

            DM-7532 added to implement this RFC, with the suggested wording as given above.

            Show
            Parejkoj John Parejko added a comment - DM-7532 added to implement this RFC, with the suggested wording as given above.

              People

              Assignee:
              Parejkoj John Parejko
              Reporter:
              Parejkoj John Parejko
              Watchers:
              Colin Slater, David Reiss, John Parejko, John Swinbank, Jonathan Sick, Kian-Tat Lim, Mario Juric, Michael Wood-Vasey [X] (Inactive), Nate Pease [X] (Inactive), Paul Price, Robert Lupton, Russell Owen, Simon Krughoff, Tim Jenness
              Votes:
              0 Vote for this issue
              Watchers:
              14 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:
                Planned End:

                  Jenkins Builds

                  No builds found.