for review: term browser (version one)

[Bert THOMPSON]

Fergus Henderson <fjh at cs.mu.OZ.AU> writes:

|I think we really need two different interfaces to the browser:
|an interactive one, and a non-interactive one which just displays
|terms with some kind of fixed depth limit.

Absolutely. I just forgot to include that one.

|"XXX" is supposed to indicate something that should be done,
|rather than merely something that could be done.

What then should be used to indicate possible improvements?
My own preference would be to use more Xs for more important revisits.
For example, "XX: Code formatting could be improved somewhat.",
"XXXXXXX: Bug causes venting of NaF6 tanks. Revisit?",

| > :- pred parse_path(list(token), path).
| > :- mode parse_path(in, out) is semidet.
| > 	% SICStus is forgiving in the syntax of paths, hence so are we.
| > 	% XXX: Be less forgiving?

|Yes, I think it would better to be less forgiving.
|If the user types something syntactically incorrect,
|then it's probably more helpful to print an error message
|than to ignore the error.

Errors don't get ignored. The syntax is just very generous.
Any initial "/" is significant and then any numbers and dotdots.
Everything else is ignored. You don't even have to use slashes.
Apart from dotdots and slashes, SICStus does precisely this.
How compatible do you want the syntax to be with SICStus?

Unix syntax is unnecessary since `directory' names can only have digits
in them, so pretty much anything else other than initial slash, dotdot,
and number can act as a directory separator.

Initially I agreed with you, but it is actually easier to type
"cd 2 3 2 2" than to type "cd 2/3/2/2". If you spend a lot of time in
a debugger, it makes a difference. Nonetheless, I probably should
disallow things like "cd orange 2 3 papaya 2 quandong rhambutan"

| > New file: frame.m
| > 	% XXX: Make frame type abstract instead?
| > 	% XXX: I'd rather not fully module-qualify the frame type.
| > % :- type frame.
| > :- type frame == list(string).

|I don't understand the second XXX comment.  Could you elaborate, please?

I simply meant that I'd rather type "frame" everywhere instead
of "frame__frame". I'll just nuke that comment.

| > 	% Clip a frame to the rectangle ((0,0),(X,Y)) where
| > 	% origin is on the top-left. Coordinate axes go down and right.
| > 	% XXX: Any reasons to implement a general clip?
| > frame__clip(X-Y, Frame, ClippedFrame) :-
| > 	list__take_upto(Y, Frame, YClippedFrame),
| > 	list__map(left(X), YClippedFrame, ClippedFrame).

|The "XXX" comment here isn't clear to me.  Could you please elaborate?

I was musing whether it's worthwhile implementing a clip pred that
allows an arbitrary cliprect rather than one constrained to start at
the origin. This might be useful if you wanted to pan around on the
entire term frame, for example. On reflection, I think this'd be overkill,
so I'll just remove the comment.

|Apart from that, it looks OK to me.  But could you please post another diff
|and/or a relative diff after you've addressed those changes?

All of your comments were good and I'm acting on them all, but let me
know what you think of the path syntax in light of what I said above.
I'll then post the relative diff.

Cheers,
Bert