ENB: The bane of code: evil "if" statements

[Edward K. Ream]

This is an Engineering Notebook post. Feel free to ignore unless you are a dev.

This post describes another surprising rabbit hole associated with recursive imports. When you are up to your ass in alligators, it's hard to remember that your original intention was to drain the swamp...

Evil "if" statements arise innocently, but they can create code that is very difficult to understand and maintain. Not all if statements are evil, but some are real doozies.

At present, I am wrestling with code in ic.init_import.  This method is a symptom that the import code is way too complex. That by itself isn't too surprising.  import code is inherently tricky.

But the "if" statement that starts with:

    if not atAuto and kind != '@auto':

is an abomination. It bit me anew when I changed the atAuto keyword arg to ic.createOutline. When combined with keyword bool arguments, this "if" statement is mind-boggling. The flags set by this method strongly effect ic.create_top_node, as I have learned to my horror ;-)

Here, the solution might be to eliminate the "if" statement entirely by requiring one or more new keyword args to ic.createOutline.  This would require more work in some (all?) callers of ic.createOutline.  That's may be best: the callers know (or should know) what they are doing ;-)

Edward

[Edward K. Ream]

​​

On Wed, Apr 5, 2017 at 11:00 AM, Edward K. Ream <edre...@gmail.com> wrote:

This is an Engineering Notebook post. Feel free to ignore unless you are a dev.

This post describes another surprising rabbit hole associated with recursive imports.

​Thinking about this in the shower I realized that all the troubles arose from the desire to eliminate duplicate code.  But some of the code couldn't be duplicated, so I added keyword args to ic.createOutline. These keyword args are the culprits behind the wretched "if" statements.

The new plan is for all callers of ic.createOutline to set up the root node for the import, and then call a simper version of ic.createOutline, say, ic.importIntoNode.​  This has a good chance of working. All experiments will be done in the new "imports" branch.

In general, eliminating common code can be a good idea or a miserable one.  Each new switch is a step towards miserable, so constant re-evaluation is a good idea.  In practice, however, this re-evaluation only happens when the code becomes unbearably hard to change :-)

Edward

[Edward K. Ream]

On Wednesday, April 5, 2017 at 12:10:51 PM UTC-5, Edward K. Ream wrote:

Thinking about this in the shower I realized that all the troubles arose from the desire to eliminate duplicate code. 

A cff reveals that only three methods call ic.createOutline. Two are in the atFile read logic: at.readOneAtAutoNode and at.importAtShadowNode. Presumably these do approximately the same things: restore existing @auto or @shadow nodes. But maybe they too create top-level nodes.  The other call is ic.importFilesCommand. It needs to create a new top-level node.

Depending on context, all three methods implement undoable commands. The undo logic seems to be the reason why code is shared.

Because there are so many flags involved, it's difficult to know exactly what the calls do.  Traces should reveal that.

Imo, the keyword args are not the primary culprits.  Indeed, new keyword args may simplify and clarify matters.  What we want are are simple if statements, depending on at most one keyword arg.

Edward

[Edward K. Ream]

On Wednesday, April 5, 2017 at 12:10:51 PM UTC-5, Edward K. Ream wrote:

​The new plan is for all callers of ic.createOutline to set up the root node for the import, and then call a simper version of ic.createOutline.

This has been a great success.  Rev b11b62 of the "import" branch contains the present code.

More simplifications are coming, as described later, but first let me recap what has already happened.

1. All the horrible logic in ic.init_import was actually a test that ic.createOutline had been called from ic.scannerUnitTest(!). So the fix was to create a new helper, ic.compute_unit_test_kind, called only from ic.scannerUnitTest. Bye, bye, evil "if" statement.

2. ic.createOutline no longer creates any node.  Two callers, ic.scannerUnitTest and ic.importFilesCommand now create a node if required.  The code is straightforward.  As an additional bonus, only ic.importFilesCommand supports undo, which speeds up unit tests a bit.

To do: eliminate the atAuto switch

Last night I went to bed thinking that this would be the end of work in the branch.  But when I awoke I saw an opportunity to eliminate what has become a mysterious blemish in Leo's API, namely various atAuto switches that appear unexpectedly in Leo's code. Eliminating this "huh?" is way too good to ignore.

After the recent simplifications, the atAuto keyword arg to ic.createOutline can now be seen to have two quite different purposes:

A. The atAuto keyword arg is passed to ic.setEncoding, which uses c.config.default_at_auto_file_encoding as a default.

B. ic.init_import uses the atAuto and atShadow keyword args to create error messages. That's the only use for the atShadow keyword arg!

This suggests that the atAuto and atShadow keyword args should be replaced by args that more clearly indicate how they are used, namely "encoding" and "errorKind" args.

Important: the atAuto switch is no longer horribly confusing in the direct callers of ic.createOutline.  However, it becomes extremely confusing (the dreaded "huh?") when back propagated to the callers of ic.scannerUnitTest.  This ends up "infecting" all unit tests for importers with a mysterious atAuto keyword.

In short, the atAuto switch started life as something innocent, but became mysterious later as the result of being back propagated to more and more methods.  It became an infection. It will be worth considerable work to get rid of this "huh?" in the code.

Edward

[Edward K. Ream]

On Thursday, April 6, 2017 at 4:06:04 AM UTC-5, Edward K. Ream wrote:

To do: eliminate the atAuto switch

...

After the recent simplifications, the atAuto keyword arg to ic.createOutline can now be seen to have two quite different purposes:

A. The atAuto keyword arg is passed to ic.setEncoding, which uses c.config.default_at_auto_file_encoding as a default.

B. ic.init_import uses the atAuto and atShadow keyword args to create error messages. That's the only use for the atShadow keyword arg!

A big oops:  There are two more important uses of atAuto in ic.createOutline:

C. ic.createOutline passes this mystery arg to the importers (the computed func).

The atAuto arg to the importers is a big "huh?".  Let's see how it affects the importers.

A cff reveals 26 uses of atAuto in the importer plugins.  Happily, all but one involving setting the atAuto ivar in the importer classes.  Heh.  This a prime example of infection.

The only "real" use of the atAuto flag is in i.gen_ref. It contains this code:

    if self.is_rst and not self.atAuto:
        return None

Again, this is a prime example of "huh?"  The next step will be to see if we can get rid of this flag. I sure hope so.  If not, it may be possible to get the effect in some clearer way.

D. If atAuto is True, ic.createOutline calls at.rememberReadPath. The actual code is:

    if atAuto:
        # Fix bug 488894: unsettling dialog when saving Leo file
        # Fix bug 889175: Remember the full fileName.
        c.atFileCommands.rememberReadPath(fileName, p)

Clearly, this code can not be changed lightly.  It will be important to track down how all callers of ic.createOutline set the atAuto flag.  I suspect (hope) that unit tests don't need to set the atAuto switch, but I wouldn't be much...

So the infection and mysteries deepen and continue. This only makes it more worthwhile to clean the code as much as possible.

Edward

P. S. I have been stung recently by criticisms that Leo's code is too complex.  Sure, parts of it are too complex.  But this thread shows the dangers of naively trying to change things.  It takes a lot of time and care. Here, the time is well spent, but I am not likely to spend more than one more day on this project.

EKR

[Edward K. Ream]

On Thursday, April 6, 2017 at 4:34:07 AM UTC-5, Edward K. Ream wrote:

> A big oops:  There are two more important uses of atAuto in ic.createOutline: [snip]

A giant step forward.  All unit tests pass when ic.createOutline asserts atAuto is False.

To make this happen, ic.scannerUnitTest calls ic.createOutline(...atAuto=True), regardless of its atAuto keyword arg.

This means:

1. All atAuto args to ic.scannerUnitTest can be eliminated.

This is a big deal: all mysterious atAuto args in the unit tests themselves will disappear.

2. All atAuto args to all importers can be eliminated.

Another big deal. This was a major "huh?".

3. The following code in i.gen_ref never fires and can be eliminated:

    if self.is_rst and not self.atAuto:
        return None

Not a huge deal, but it's always good to remove a mysterious test and special case.

We are about to see the end of a major "huh?" that has infected Leo's code.

Edward

[Edward K. Ream]

On Thursday, April 6, 2017 at 4:51:11 AM UTC-5, Edward K. Ream wrote:

> 1. All atAuto args to ic.scannerUnitTest can be eliminated...

> 2. All atAuto args to all importers can be eliminated.

> 3. The following code in i.gen_ref never fires and can be eliminated:...

> We are about to see the end of a major "huh?" that has infected Leo's code.

All completed at rev e2b2291. This is a huge collapse in complexity in Leo's core.

I plan to merge the "imports" branch into the trunk fairly soon. There is nothing to be gained by avoiding widespread testing.

There are some mysterious instances of atAuto constants in leoAtFile.py.  I might look at removing them as part of fixing bugs in the javascript importer. But no guarantees. I suspect that these switches really are essential. Breaking the code in leoAtFile.py would be catastrophic.

Edward