7 Oct
2003
7 Oct
'03
10:51 p.m.
Hi John,
thanks a lot for your review! I really appreciate it, as you are the
first one to report real bugs and to give lots of useful hints and
suggestions. I am just writing this as kind of a log while looking at
my code and trying to solve the problems.
*) The Folder>>defaultDocument method adds a
Code item instead of a
Paragraph -- there's a missing ";".
*) SwazooServer>>start has hard coded the ip to be '*'.
Those bugs are fixed.
*) The tests don't have great coverage. I had to
fix several bugs
after all
tests passed.
[...]
BTW, you could use my coverage tool to improve the tests. Currently, it
shows about 60% method coverage for the tests.
Wow, that is really a great tool, I didn't knew about it. I will used
it in the feature more regularly when writing tests.
*) You can't save the Syntax page after you edit
it. This occurs in VW
also.
It appears that it is interpreting the "*" inside the
"<code>*</code>".
Yeah, that is a know problem: I build the parse-tree manually, because
one would need to escape all the special characters like *, [, etc.
using &xxx; Right now this is not on top of my to-do-list, but I will
fix that some time.
*) When you have an exception handler, the exception
block shouldn't
just
return since that isn't legal ANSI Smalltalk. For example, instead of
having
"[self something] on: Error do: [:ex | 5]" it should be "[self
something]
on: Error do: [:ex | ex return: 5]".
[...]
*) I found a few non-#return: exceptions while running the tests, but I
found others after. I finally ran the rewrite rule:
``@a
on: ``@b
do: [:`var | ``@c
`{:node | node isMessage not
or: [node receiver ~= `var]}]
->
``@a on: ``@b do: [:`var | `var return: ``@c]
Most of these showed up as bugs in the permissions. For example, I
would get
a message saying that I couldn't do something, but it would also show
me the
stuff that I wasn't supposed to do. For example, I could see the
history or
edit blocks when I wasn't logged in.
Didn't know about that, the way I wrote it seemed to me more natural.
But if this is the ANSI standard, I will change it of course. Thanks
for providing the rewrite rule, this is really a great tool to use for
such kind of refactorings. However the rule did not detect that piece
of code, what should have been probably also rewritten:
^[ WikiParser parse: input readStream for: nil ]
on: Refactory.SmaCC.SmaCCParserError
do: [ :error |
Transcript show: 'WARNING: Unable to parse '; show: aClass name; cr.
nil ]
I didn't notice the problem with the permissions and could not
reproduce in VisiualWorks. Is this only a problem with #Smalltalk?
*) Does the #nextVersionBecome: message ever get sent
where the
argument's
class isn't the same as the receiver's class? I didn't see any places
where
that would happen so I changed it to be:
nextVersionBecome: aStructure
| previous |
previous := self copy.
self instanceVariableNames do: [:each | self instanceVariableNamed:
each put: (aStructure instanceVariableNamed: each)].
self version: previous version + 1.
self predecessor: previous.
^self
This version gets around having to #become: the versions.
That makes the tests pass, but unfortunately the real issue doesn't
seem to be covered by the tests. There might be links with references
to that structure anywhere within the wiki that have to be updated. The
cleanest solution (but maybe a bit slow) I can think of, would be to
write a visitor walking through the documents and all its versions of
the whole wiki and check manually all the internal links. As I have a
test case for that behavior now, it should be easy to get rid of the
one and only #become: in SmallWiki ...
*) Here's an ANSI version of the
moveDown:ifError:
moveDown: anObject ifError: aBlock
| index temp |
index := self indexOf: anObject.
(index between: 1 and: self size - 1)
ifTrue: [ temp := self at: index.
self at: index put: (self at: index + 1).
self at: index + 1 put: temp ]
ifFalse: [ aBlock value ]
#swap:with: and #find: are VW messages (probably Squeak also has them,
but I
don't think VA or Dolphin does).
Ok, I replaced the old code with your suggestion and added two test to
check if it is really working as expected.
*) It would be nice if the integer comparisons used #=
instead of #==.
In
#Smalltalk, "1 + 2 == 3" is false since I have to use real objects for
integers.
Now I know that it is stupid to use #== when it wouldn't be needed.
When starting with the project I had a different opinion, that is why
there are still a lot of unnecessary #== in the code. I hope that I
cleaned everything now ...
*) The VisitorRendererWiki>>contents message
assumes that "stream cr"
only
puts one character in the string. It would be better if the #cr wasn't
put
on the stream to begin with so we didn't need the #copyFrom:to:.
Well, this is just a dirty trick to make the parser work. As the
wiki-syntax is line-based, I basically parse every line on its own up
to the line-ending. Therefor I have to make sure that also the last
line ends with a #cr. Another problem is, that the parser does not
allow nested lists. I couldn't figure out how to parse this properly.
The visitors however, would be able to render it correctly.
#1
##1.1
##1.2
#2
#-2.1
#-#2.1.1
I am usually learning from examples, but I couldn't find a wiki-grammar
anywhere on the web, so this is probably the first one ever written.
Furthermore I must admit that I wrote the parse before I had any
lecture about scanners, parsers and compilers, therefor there are even
more such ugly things that might have been written in a much nicer way.
If I find some time in the future I might try to rethink the whole
parsing and look if I manage to make it better.
*) The Folder>>remove: method assumes that
#remove: throws an error if
the
item isn't in the collection -- the ANSI standard says that the
behavior is
undefined, so #Smalltalk doesn't throw an error. Anyway, I doubt that
it is
really necessary, but there are a few tests for the error.
Ok, I changed the code and raise the exception manually if necessary.
*) The tests didn't test evaluating code that
returns a block and then
doing
a renderOn: that block. In #Smalltalk, it just evaluates the parse
trees
instead of compiling the code so evaluated code blocks are instances of
RBFakeEvaluationBlock instead of BlockClosure. When I loaded the
initial
page, it displayed "a RBFakeEvaluationBlock" instead of the items in
the
collection.
Are you taking about code in the wiki-documents? I haven't thought
about returning blocks and don't even know what this could be useful
for? So instead of writing something like
Current time is: [ [ :h | h render: Timestamp now ] ]
you can always rewrite as
Current time is: [ html render: Timestamp now. nil ]
or even shorter
Current time is: [ Timestam now ]
These lines are all equivalent in VisualWorks, but I don't know about
#Smalltalk. You must have seen that I am also using dirty tricks with
the thisContext to setup global variables for that block. How do you
manage that? Would you prefer just to have the current action passed as
a parameter, even-tough the code written by the user would be longer in
case he didn't want just to return a value?
Current time is: [ :action | action html render: Timestamp now. nil ]
*) The PageEdit>>exception: method wasn't
covered in the tests. It
sends
#nextAvailable: which isn't implemented in #Smalltalk. I implement
#next: to
work that way. The ANSI standard says that the behavior is undefined
when
the integer is larger than the amount available, so I defined it to
just
return the remaining elements.
Ok, that has been fixed. A long time ago I decided not to cover the
action-package with tests. However, time has changed and there is a lot
of code that is somehow critical, I probably have to write some tests
in that area too.
*) The RecentChanges>>renderDate:changes: method
isn't covered.
#Smalltalk
uses the ANSI DateAndTime and Duration classes. It doesn't have the
Date,
Time & Timestamp classes like VW. Anyway, the #asTime method isn't
implemented in #Smalltalk.
Unfortunately there is no class DateAndTime and Duration in VisualWorks
7.1, so I cannot fix that problem easily. In that case Squeak seems to
have better ANSI comparability than VisualWorks.
*) TemplateEdit>>repositoryIndex wasn't
covered. #Smalltalk doesn't
have
#upToSeparator. I changed it to be #upTo: a space.
Ok, I implemented it this way too.
*) PageHistory>>renderDocument: sends
#contractTo: which wasn't
implemented
in #Smalltalk.
I am using a more portable form of code there ...
BTW, are you using the latest version of SmaCC? I put
one on the public
archive a week or two ago. It fixes a bug in the scanner generator
where
some token might be scanned correctly. For example, if you had
something
like:
<aa> : aa;
<anything> : .;
It might scan "aa" as <anything> <anything> instead of <aa>.
In
addition to
that fix, I also explicitly put it under the MIT license.
Yeah I noticed that, because Serge Stinckwich reported failing
parser-tests when downloading SmallWiki from the Cincom Repository. I
checked out the new version and had to tweak some minor things to make
all the tests pass again. However the new SmaCC version had never been
a problem when editing the page from the web.
Today I have just published the changes to the SCG repository and I
will push them to the Cincom Public repository tomorrow evening, after
having written some more additional tests.
Regards,
Lukas
--
Lukas Renggli
http://renggli.freezope.org