Statistics on plan.py --------------------- Solo verified by Shipman using trace tables: see plan.vrf for the trace tables themselves. Program size: 205/891 code lines, 23% non-blank non-comment lines. ================================================================ Syntax errors ---------------------------------------------------------------- S1 Stray block of text, apparently an older version of the I/F from the doc string of Plan()'s constructor, deleted from end of the source file. ---------------------------------------------------------------- S2 In a formal parameter list, one cannot have a non-keyword argument after a keyword argument. Therefore the constructor for Plan() must be changed from: def __init__ ( self, fileName=None, pathMap ): to: def __init__ ( self, pathMap, fileName=None ): necessitating changes in pystyler.py as well as planview.cgi. ---------------------------------------------------------------- S3 Neglected to import template, since we now need a template pool; and tree, from my standard library. ---------------------------------------------------------------- S4 Declarations of BODY_EXT and HTML_EXT moved from pystyler.py to this module. One erroneous reference to HTML_EXTENSION fixed. ---------------------------------------------------------------- S5 This module must import the body.py module. ---------------------------------------------------------------- ================================================================ Bugs that would be caught in a more strongly-typed language ---------------------------------------------------------------- T1 In Plan.__read(), this method call: if ( ( not scan.atendLine() ) and should be: if ( ( not scan.atEndLine() ) and ---------------------------------------------------------------- T2 Plan.__adjustTopicStack() needs scan for error reporting ---------------------------------------------------------------- T3 Forgot the `self' argument to Topic.fullPath(). ---------------------------------------------------------------- T4 This module cannot access `args' since that object is a global within another module (pystyler.py). Therefore, the Topic.expandTree() method must get the two relevant members, args.force and args.outRoot, and pass them down its subtree. ---------------------------------------------------------------- T5 Plan.__addTopic() must be passed the `scan' object so it can point to the source line when logging errors. ---------------------------------------------------------------- T6 Neglected to import `os'. ---------------------------------------------------------------- ================================================================ Logic bugs ---------------------------------------------------------------- B1 No one ever initialized Plan.shortNameMap. In __init__(), it was delegated to .__read(): #-- 4 -- # [ if fileName names a readable, valid plan file -> # ... # self.shortNameMap := as invariant # ... # else ... ] self.__read ( fileName ) The overall I/F for .__read() included this same line, but its third prime assumed that the element had been initialized: #-- 3 -- # [ if topicStack is a list of the path of nodes in the topic tree # from the root (if any) to the previous topic (if any) -> # ... # self.shortNameMap +:= new entries added mapping shortName # |-> topic for valid topics from the # remainder of scan # ... In the trace table file, the relevant case is: #### Case 5: fileName is readable and valid I had the same mental lapse in verification as I did in writing. State before step 3: topicStack: [] self.root: None errCount: @(Log().count()) scan: a Scan object pointing at start of fileName After step 3, the relevant line is: self.shortNameMap: @+(entries mapping short name |-> topic from fileName) Suggestion for future improvement: instead of just saying `as invariant', copy the actual invariant into the I/F, and perhaps I'll notice the lack of a value. When I wrote __init__(), I probably should have created the dictionary there. This also points to another weakness of my method: before the constructor is completed (which may involve many sub-methods), I have a tendency to play fast and loose with partially established class invariants. ---------------------------------------------------------------- B2 Another bug very similar to B1. Again, I had a problem with partially established class invariants in routines under the constructor. In Plan.__init__(), I never established the invariant: .tmplStack: [ a list of one or more Template objects such that the first is the default template and the rest represent the stack of templates pushed by $template commands in the plan file and not yet popped by empty $template commands ] In fact, the stack was empty. ---------------------------------------------------------------- B3 In Plan.__read(), the test for properly balanced $template control lines is that the final size of self.tmplStack is 1, not 0---due to the default template at the base of the stack. ---------------------------------------------------------------- B4 In Plan.__read(), blank lines are being flagged as invalid. This condition won't work: if ( ( not scan.atEndLine() ) and ( scan.tabMatchArb ( CONTROL_LINE_CHAR ) ) ): self.__controlLine ( scan ) else: self.__buildTopic ( scan, topicStack ) Should be: if not scan.atEndLine(): if scan.tabMatchArb ( CONTROL_LINE_CHAR ): self.__controlLine ( scan ) else: self.__buildTopic ( scan, topicStack ) ---------------------------------------------------------------- B5 Plan.__buildTopic(): The checking and trimming of topicStack suffers from off-by-one errors that may result from translating the Icon version, since Icon has origine-one indexing. The root problem is that rawTopic.depth is expressed in the number of stars, but some calculations seem to assume that depth is 0 for the root, not 1. The cure for this affects several areas: - In class RawTopic, change the definition of the depth element so 0 means root, 1 means child of root, etc. - In Plan.__buildTopic(), rewrite prime #2 using zero-origin depth. - In Plan.__parseTopic(), subtract one from number of stars to get depth passed to RawTopic() constructor. - In Plan.__adjustTopicStack, same changes as previous step. - In class Topic, clarify that depth is 0-origin. ---------------------------------------------------------------- B6 In Plan.__adjustTopicStack, this line does not change the caller's topicStack: topicStack = topicStack[:newDepth] but this does: del topicStack[newDepth:] ---------------------------------------------------------------- B7 In Plan.__controlTemplate, failed to skip spaces after the control word. ---------------------------------------------------------------- B8 Topic.__pageUpToDate() does not check the timestamp of the effective template. ---------------------------------------------------------------- B9 Topic.__creDir() complains if the file is a link. But when creating an absolute path name at the TCC, directories like /u/john are links, so assume that an existing link must be a directory. If it's a file link, that will show up when we try to create .html files in it, so the effect of this change is only to delay detection of errors, not to fail to detect them. ---------------------------------------------------------------- B10 In Plan.lookupShortName(), I went to the trouble of splitting the anchor out and then used the unsplit version. This: topic = self.shortNameMap[shortName] should be: topic = self.shortNameMap[pathPart] ---------------------------------------------------------------- B11 Kathy Hedges found this bug in Topic.relpath(): If in an RR tag a page "foo.g" links to one of its own anchors, such as "foo#anch", the resulting tag has a URL of ".html#anch", not "foo.html#anch". Explanation: Topic.relpath() breaks the source and target path into pieces and then removes all matching leading elements. In the given case, both lists start out as ["foo"], so after this deletion both lists are empty. The fix is to prevent removal of the final element. Old code: while ( ( len(fromList) > 0 ) and ( len(toList) > ) ) and ( fromList[0] == toList[0] ) ): del fromList[0] del toList[0] New code: while ( ( len(fromList) > 1 ) and ( len(toList) > 1 ) and ( fromList[0] == toList[0] ) ): del fromList[0] del toList[0] ----------------------------------------------------------------