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

            No builds found.
            Parejkoj John Parejko created issue -
            Hide
            ktl Kian-Tat Lim added a comment -

            +0.2: I generally lean in favor but take no strong position.

            Show
            ktl Kian-Tat Lim added a comment - +0.2: I generally lean in favor but take no strong position.
            Hide
            rowen Russell Owen added a comment -

            +1 for the reasons given.

            I admit that using if False:... does allow a linter to check the code, but it is still likely to rot over time, so this doesn't provide much help and it really hurts readability.

            I also really like John Parejko's suggestion that disabled code must include a comment with a clear timeline for when it will be removed – that seems useful for all our code (e.g. C++ as well).

            Show
            rowen Russell Owen added a comment - +1 for the reasons given. I admit that using if False:... does allow a linter to check the code, but it is still likely to rot over time, so this doesn't provide much help and it really hurts readability. I also really like John Parejko 's suggestion that disabled code must include a comment with a clear timeline for when it will be removed – that seems useful for all our code (e.g. C++ as well).
            Hide
            rhl Robert Lupton added a comment -

            -0.4; being able to add and remove functional code is useful, and an overload of commenting. Using """ ... """ would be better, I suppose, but it's also an overload. if False says: "don't run this valid code".

            I'm OK with banning if False/True if we have a way of specifying how to turn it on (cf. lsstDebug).

            I do not like the proposal for a comment on when code will be removed. Dead code should be removed immediately, but code that is explicitly included but not run (e.g. debug or profiling code) may have arbitrarily longevity.

            Show
            rhl Robert Lupton added a comment - -0.4; being able to add and remove functional code is useful, and an overload of commenting. Using """ ... """ would be better, I suppose, but it's also an overload. if False says: "don't run this valid code". I'm OK with banning if False/True if we have a way of specifying how to turn it on (cf. lsstDebug). I do not like the proposal for a comment on when code will be removed. Dead code should be removed immediately, but code that is explicitly included but not run (e.g. debug or profiling code) may have arbitrarily longevity.
            Hide
            Parejkoj John Parejko added a comment -

            Dead code should be removed immediately, but code that is explicitly included but not run (e.g. debug or profiling code) may have arbitrarily longevity.

            What distinguishes "dead code" from "debugging code"? The latter seems to be why you advocate for if False:, but that looks the same as "dead code" to me.

            Show
            Parejkoj John Parejko added a comment - Dead code should be removed immediately, but code that is explicitly included but not run (e.g. debug or profiling code) may have arbitrarily longevity. What distinguishes "dead code" from "debugging code"? The latter seems to be why you advocate for if False: , but that looks the same as "dead code" to me.
            Hide
            rhl Robert Lupton added a comment -

            dead code is code that has been replaced. debugging code is code that you may want back if problems show up, due to unexpected features in the data (or bugs; but I hope the former)

            Show
            rhl Robert Lupton added a comment - dead code is code that has been replaced. debugging code is code that you may want back if problems show up, due to unexpected features in the data (or bugs; but I hope the former)
            Hide
            price Paul Price added a comment -

            I am strongly opposed to leaving commented-out code in a file. Commented-out code is subject to the same bitrot that affects comments and docstrings. We have a version control system for restoring code from the past.

            I like RHL's proposal: dead code should be removed immediately, while debugging code may be permitted in an if block.

            If we are going to allow "dead code", I am very much in favour of putting the code in an if block:

            • The scope of commented-out code is not clear (requires the equivalent of an END statement which can easily be misplaced and is very un-pythonic), while indenting the code in an if block makes the scope immediately clear, and allows for multiple unrelated dead code sections in succession.
            • I consider the argument that "you have to visually parse the if block to realize it is unused" to be weak — if you want to understand any code, you have to visually parse the block. if False or if debug stands out quite clearly and is very simple to parse.
            • The fact that the "dead code" still gets compiled is an advantage, not a weakness.
            • The consistency with C++ is an advantage.
            Show
            price Paul Price added a comment - I am strongly opposed to leaving commented-out code in a file. Commented-out code is subject to the same bitrot that affects comments and docstrings. We have a version control system for restoring code from the past. I like RHL's proposal: dead code should be removed immediately, while debugging code may be permitted in an if block. If we are going to allow "dead code", I am very much in favour of putting the code in an if block: The scope of commented-out code is not clear (requires the equivalent of an END statement which can easily be misplaced and is very un-pythonic), while indenting the code in an if block makes the scope immediately clear, and allows for multiple unrelated dead code sections in succession. I consider the argument that "you have to visually parse the if block to realize it is unused" to be weak — if you want to understand any code, you have to visually parse the block. if False or if debug stands out quite clearly and is very simple to parse. The fact that the "dead code" still gets compiled is an advantage, not a weakness. The consistency with C++ is an advantage.
            Hide
            wmwood-vasey Michael Wood-Vasey added a comment -

            +1

            I think there are several categories of such blocks that people are thinking about:
            1. Notes to myself
            2. Snippets that are useful to me to debug what's going on here
            3. Something I tried, but decided to use the other thing instead, but am still dithering about what to use. This happens understandably when prototyping code, but should not be in mainstream code: anything in lsst_distrib.

            My implementation proposal, would be that Type #1 can be rephrased as comments. Type #2 and #3 can be wrapped as follows. Replace:

            if False:
            

            with

            DEBUG_PLOTTING_OF_FOO = False
            if DEBUG_PLOTTING_OF_FOO:
            

            And that additionally for Type #3 cases, it may be more clear to break things up into distinct functions.

            Show
            wmwood-vasey Michael Wood-Vasey added a comment - +1 I think there are several categories of such blocks that people are thinking about: 1. Notes to myself 2. Snippets that are useful to me to debug what's going on here 3. Something I tried, but decided to use the other thing instead, but am still dithering about what to use. This happens understandably when prototyping code, but should not be in mainstream code: anything in lsst_distrib . My implementation proposal, would be that Type #1 can be rephrased as comments. Type #2 and #3 can be wrapped as follows. Replace: if False: with DEBUG_PLOTTING_OF_FOO = False if DEBUG_PLOTTING_OF_FOO: And that additionally for Type #3 cases, it may be more clear to break things up into distinct functions.
            Hide
            jsick Jonathan Sick added a comment - - edited

            I like Michael's idea and I'll expand on it. If there's potentially useful debugging code inside a method or function, then why not turn it into a real feature of the method/function? By that I mean, why not have a boolean keyword arg in the method/function, or a module-level flag variable, or even an environment variable flag? That way the debugging capability could be documented in the docstring so that, in the future, someone knows this debugging facility even before digging into the source code. If the debugging code is non-trivial, it could even be included in a unit test so that it doesn't bit rot.

            (And yes, I agree with the gist of the RFC that inert if Flase: statements ought to be systematically refactored into a better system).

            Show
            jsick Jonathan Sick added a comment - - edited I like Michael's idea and I'll expand on it. If there's potentially useful debugging code inside a method or function, then why not turn it into a real feature of the method/function? By that I mean, why not have a boolean keyword arg in the method/function, or a module-level flag variable, or even an environment variable flag? That way the debugging capability could be documented in the docstring so that, in the future, someone knows this debugging facility even before digging into the source code. If the debugging code is non-trivial, it could even be included in a unit test so that it doesn't bit rot. (And yes, I agree with the gist of the RFC that inert if Flase: statements ought to be systematically refactored into a better system).
            Hide
            ctslater Colin Slater added a comment -

            In discussion with John Parejko I have attempted to summarize his intent with this RFC into three different components:

            1. Commented blocks are stylistically preferable over if False blocks.
            2. The stack has numerous instances of disabled code that could be removed
            3. When code is disabled, it should receive some appropriate indication of why it was disabled

            Of these, I think #3 enjoys the most agreement on the basis that named things are usually better than unnamed things. Michael Wood-Vasey's proposal fits in with this well. Item #2 is also probably uncontroversial in the abstract, but is not really an actionable task for an RFC other than to remind everyone to consider these blocks carefully when checking them in, or to create some policy statement about when these blocks are permissible (e.g. for debugging code). That leaves item #1 as a largely stylistic issue, which I think is the most debatable aspect of this RFC. My own feeling is that neither block comment style is significantly better than the other, so I would give this -0.2 for unnecessary standardization. However, if debugging blocks are really the only such code that should exist, then I think it would be much preferable to leave these as if DEBUG statements than commented blocks.

            Show
            ctslater Colin Slater added a comment - In discussion with John Parejko I have attempted to summarize his intent with this RFC into three different components: 1. Commented blocks are stylistically preferable over if False blocks. 2. The stack has numerous instances of disabled code that could be removed 3. When code is disabled, it should receive some appropriate indication of why it was disabled Of these, I think #3 enjoys the most agreement on the basis that named things are usually better than unnamed things. Michael Wood-Vasey 's proposal fits in with this well. Item #2 is also probably uncontroversial in the abstract, but is not really an actionable task for an RFC other than to remind everyone to consider these blocks carefully when checking them in, or to create some policy statement about when these blocks are permissible (e.g. for debugging code). That leaves item #1 as a largely stylistic issue, which I think is the most debatable aspect of this RFC. My own feeling is that neither block comment style is significantly better than the other, so I would give this -0.2 for unnecessary standardization. However, if debugging blocks are really the only such code that should exist, then I think it would be much preferable to leave these as if DEBUG statements than commented blocks.
            Hide
            krughoff Simon Krughoff added a comment -

            I am like the statements by Jonathan Sick and Michael Wood-Vasey. I think it would be very nice for code with debugging blocks to take a debug keyword argument, but that seems out of scope for this RFC.

            Show
            krughoff Simon Krughoff added a comment - I am like the statements by Jonathan Sick and Michael Wood-Vasey . I think it would be very nice for code with debugging blocks to take a debug keyword argument, but that seems out of scope for this RFC.
            Hide
            rhl Robert Lupton added a comment -

            By that I mean, why not have a boolean keyword arg in the method/function, or a module-level flag variable, or even an environment variable flag?

            Jonathan Sick Do you know about https://lsst-web.ncsa.illinois.edu/doxygen/x_masterDoxyDoc/base_debug.html (the main problem with this is the lack of discoverability). It's enabled with --debug

            Show
            rhl Robert Lupton added a comment - By that I mean, why not have a boolean keyword arg in the method/function, or a module-level flag variable, or even an environment variable flag? Jonathan Sick Do you know about https://lsst-web.ncsa.illinois.edu/doxygen/x_masterDoxyDoc/base_debug.html (the main problem with this is the lack of discoverability). It's enabled with --debug
            Hide
            jsick Jonathan Sick added a comment -

            Robert Lupton: I mean nothing more elaborate than

            def myFunction(*args, plotDiagnostics=False):
                # ...
                if plotDiagnostics:
                    # ... debugging code (e.g., a diagnostic plot)
            

            If lsstDebug is the stack's idiomatic way of achieving this, then that's perfect.

            My intent is merely to say that if this code is important enough to leave in the codebase, it ought to be a useable feature that can be discovered/activated without having to read the source. I think this will be important in the future when users are have peculiar datasets and are having trouble applying some of our code; I imagine they'd be delighted if there's a feature they can activate to provide diagnostic information.

            Show
            jsick Jonathan Sick added a comment - Robert Lupton : I mean nothing more elaborate than def myFunction( * args, plotDiagnostics = False ): # ... if plotDiagnostics: # ... debugging code (e.g., a diagnostic plot) If lsstDebug is the stack's idiomatic way of achieving this, then that's perfect. My intent is merely to say that if this code is important enough to leave in the codebase, it ought to be a useable feature that can be discovered/activated without having to read the source. I think this will be important in the future when users are have peculiar datasets and are having trouble applying some of our code; I imagine they'd be delighted if there's a feature they can activate to provide diagnostic information.
            Hide
            Parejkoj John Parejko added a comment -

            I'm totally on board with Colin's phrasing above.

            Show
            Parejkoj John Parejko added a comment - I'm totally on board with Colin's phrasing above.
            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.
            Parejkoj John Parejko made changes -
            Field Original Value New Value
            Link This issue relates to DM-7532 [ DM-7532 ]
            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.
            Parejkoj John Parejko made changes -
            Resolution Done [ 10000 ]
            Status Proposed [ 10805 ] Adopted [ 10806 ]
            Parejkoj John Parejko made changes -
            Status Adopted [ 10806 ] Implemented [ 11105 ]

              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, 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

                  No builds found.