Skip to content

Added parametrized chunks #147

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sebasguts wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Added parametrized chunks #147

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 18 additions & 5 deletions doc/Comments.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,21 +408,34 @@ not finished or temporary files.

<Subsection Label="@BeginChunk">
<Index Key="@BeginChunk"><C>@BeginChunk, @EndChunk, and @InsertChunk</C></Index>
<Heading>@BeginChunk <A>name</A>, @EndChunk, and @InsertChunk <A>name</A></Heading>
Text insider of a @BeginChunk / @EndChunk part will not be inserted into
<Heading>@BeginChunk <A>name[ param1,...]</A>, @EndChunk, and @InsertChunk <A>name[ param1_value,...]</A></Heading>
Text inside of a @BeginChunk / @EndChunk part will not be inserted into
the final documentation directly. Instead, the text is stored in an internal buffer.

That chunk of text can then later on be inserted in any other place by using the
@InsertChunk <A>name</A> command.

Chunks may optionally be parametrized, by specifying a comma-separated
list of parameter names after the chunk name given to @BeginChunk.
Each parameter name must be a valid &GAP; identifier
(and hence may only contain letters, digits and underscores).
When inserting a parametrized chunk, a comma-separated list of
parameter values must follow the chunk name given to @InsertChunk.
The number of parameter values must match the number of parameter names.
When this is done, instead of directly inserting the chunk text,
for every parameter <A>param</A>, every occurrence of the string <A>%{param}</A>
is substituted by the corresponding parameter value.

If you do not provide an @EndChunk, the chunk ends at the end of the file.
<Listing><![CDATA[
#! @BeginChunk MyChunk
#! Hello, world.
#! @BeginChunk MyChunk who
#! Hello, %{who}.
#! @EndChunk

#! @InsertChunk MyChunk
#! @InsertChunk MyChunk world
## The text "Hello, world." is inserted right before this.
## The text "Hello, whole universe." is inserted right behind this comment
#! @InsertChunk MyChunk whole universe
]]></Listing>

You can use this to define an example like this in one file:
Expand Down
4 changes: 4 additions & 0 deletions gap/DocumentationTree.gd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@
##
######################################

BindGlobal( "AUTODOC_IdentifierLetters",
"+-0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ_abcdefghijklmnopqrstuvwxyz" );
Copy link
Member

@fingolfin fingolfin Jan 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So an AutoDoc identifier is allowed to contain minus and plus? Why? What is this used for?


DeclareGlobalFunction( "AUTODOC_TREE_NODE_NAME_ITERATOR" );
DeclareGlobalFunction( "AUTODOC_LABEL_OF_CONTEXT" );
DeclareGlobalFunction( "AUTODOC_INSTALL_TREE_SETTERS" );
Expand Down Expand Up @@ -57,6 +60,7 @@ DeclareOperation( "DocumentationExample", [ IsTreeForDocumentation, IsList ] );
DeclareOperation( "DocumentationExample", [ IsTreeForDocumentation ] );
DeclareOperation( "DocumentationDummy", [ IsTreeForDocumentation, IsString, IsList ] );
DeclareOperation( "DocumentationDummy", [ IsTreeForDocumentation, IsString ] );
DeclareOperation( "DocumentationDummyInstance", [ IsTreeForDocumentation, IsTreeForDocumentationNode ] );
Copy link
Member

@fingolfin fingolfin Jan 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this name DocumentationDummyInstance? I'd never guess that it is related to parametrized chunks. Why not e.g. DocumentationParametrizedChunk?

Copy link
Author

@sebasguts sebasguts Jan 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not related to parametrized chunks. Previously, for a chunk a dummy was created, and the same dummy was inserted into the tree. Now, for a chunk a dummy is created, but each time a chunk is inserted in the tree, a dummy instance is inserted.

Copy link
Member

@fingolfin fingolfin Jan 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how you can say it is not related when clearly they are related (one is used to implement the other) :-).

Anyway: I guess I find these names very unfortunate. What's a "dummy" here? I grepped through the code, and the whole "dummy" stuff is only ever used to implement chunks. So why isn't it called "chunk"?

I am also confused as to the relation between "Dummy" and "DummyInstance". While I am pretty hopeful I could figure it out from digging through the code, I feel that (a) better names, and (b) at least some minimal comments, would go a long way to making this code more understandable (and hence maintainable).

What I gathered so far is that before this PR, DocumentationDummy is called both for creating and inserting a "chunk". Now it is only called for creating it, while DocumentationDummyInstance is called for inserting it. So, why not replace those operations with functions MakeChunk and InsertChunk, which do what their names suggest? They could then also perform the extra tests I suggest below (i.e. the former would produce an error if a chunk with that name already exists, while the latter could produce an error if no chunk with that name was declared before).

DeclareOperation( "DocumentationCode", [ IsTreeForDocumentation, IsString, IsList ] );
DeclareOperation( "DocumentationCode", [ IsTreeForDocumentation, IsString ] );
DeclareOperation( "DocumentationManItem", [ IsTreeForDocumentation ] );
Expand Down
52 changes: 48 additions & 4 deletions gap/DocumentationTree.gi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,6 @@
##
#############################################################################

##
BindGlobal( "AUTODOC_IdentifierLetters",
"+-0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ_abcdefghijklmnopqrstuvwxyz" );

DeclareRepresentation( "IsTreeForDocumentationRep",
IsAttributeStoringRep and IsTreeForDocumentation,
[ ] );
Expand Down Expand Up @@ -117,6 +113,15 @@ BindGlobal( "TheTypeOfDocumentationTreeCodeNodes",
NewType( TheFamilyOfDocumentationTreeNodes,
IsTreeForDocumentationCodeNodeRep ) );

## DocumentationDummyInstance
DeclareRepresentation( "IsTreeForDocumentationDummyInstanceNodeRep",
IsTreeForDocumentationNodeRep,
[ ] );

BindGlobal( "TheTypeOfDocumentationTreeDummyInstanceNodes",
Copy link
Member

@fingolfin fingolfin Jan 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does the type have an extra "Tree" in the name?

NewType( TheFamilyOfDocumentationTreeNodes,
IsTreeForDocumentationDummyInstanceNodeRep ) );

###################################
##
## Tools
Expand Down Expand Up @@ -263,6 +268,15 @@ InstallMethod( DocumentationDummy, [ IsTreeForDocumentation, IsString ],
return node;
end );

InstallMethod( DocumentationDummyInstance, [ IsTreeForDocumentation, IsTreeForDocumentationDummyNodeRep ],
function( tree, dummy_node )
local node;
node := rec( dummy_node := dummy_node );
ObjectifyWithAttributes( node, TheTypeOfDocumentationTreeDummyInstanceNodes,
Label, Concatenation( Label( dummy_node ), "_", String( AUTODOC_TREE_NODE_NAME_ITERATOR( tree ) ) ) );
return node;
end );

##
InstallMethod( DocumentationCode, [ IsTreeForDocumentation, IsString, IsList ],
function( tree, name, context )
Expand Down Expand Up @@ -637,6 +651,36 @@ InstallMethod( WriteDocumentation, [ IsTreeForDocumentationDummyNodeRep, IsStrea
fi;
end );

##
InstallMethod( WriteDocumentation, [ IsTreeForDocumentationDummyInstanceNodeRep, IsStream ],
function( node, filestream )
local dummy, variables, variable_values, new_content, current_variable, current_variable_value,
i, j, current_string, dummy_label;

dummy := node!.dummy_node;
dummy_label := Label( dummy );
if not IsBound( dummy!.content ) then
return;
fi;
new_content := ShallowCopy( dummy!.content );
variables := dummy!.variable_names;
variables := List( variables, i -> Concatenation( "%{", i, "}" ) );
variable_values := node!.variable_values;
if Length( variables ) <> Length( variable_values ) then
Error( "While inserting chunk ", dummy_label, ": wrong number of parameter values" );
Copy link
Member

@fingolfin fingolfin Jan 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error should not ever happen, right? Because of filtering we performed before. So, Perhaps turn this into an Assert?

Although... does the system prevent the user from re-defining a chunk? I.e. from using @BeginChunk twice with the same name, but possibly different numbers of parameters?

Copy link
Member

@fingolfin fingolfin Jan 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tried this, and you can do it (undocumented!) -- seems the chunks get concatenated. Is that intentional? I am tempted to forbid it.

Conversely, it is possible to insert a non-existing chunk, which I think will make for hilarious debugging fun. Now it's OK to insert a chunk before it is declared (so you can't check for this in @InsertChunk), but when writing it, we should IMHO detect if there isn't a chunk with that name specified. That would suggest storing for each @InsertChunk the file and line it was used in, so that the delayed error message can correctly pinpoint the problem. Alternatively, demand that chunks are declared before use? (But that's a bit tricky, because we currently do not specify in which order files are read. Which is a problem, too, I guess...)

Copy link
Member

@fingolfin fingolfin Jan 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated, but while looking through the code, I saw that there is a 3-arg version of DocumentationDummy which apparently is unused -- delete it?

fi;
for i in [ 1 .. Length( variables ) ] do
current_variable := variables[ i ];
current_variable_value := variable_values[ i ];
for j in [ 1 .. Length( new_content ) ] do
new_content[ j ] := ReplacedString( new_content[ j ], current_variable, current_variable_value );
od;
od;

WriteDocumentation( new_content, filestream );

end );

##
InstallMethod( WriteDocumentation, [ IsTreeForDocumentationExampleNodeRep, IsStream ],
function( node, filestream )
Expand Down
40 changes: 38 additions & 2 deletions gap/Parser.gi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -613,14 +613,50 @@ InstallGlobalFunction( AutoDoc_Parser_ReadFiles,
current_item!.level := Int( current_command[ 2 ] );
end,
@InsertChunk := function()
Add( current_item, DocumentationDummy( tree, current_command[ 2 ] ) );
local parameters, vars, title, inserted_dummy, position_first_space;
parameters := current_command[ 2 ];
position_first_space := Position( parameters, ' ' );
if position_first_space = fail then
title := parameters;
vars := [ ];
elif IsInt( position_first_space ) then
title := parameters{[ 1 .. position_first_space - 1 ]};
vars := parameters{[ position_first_space + 1 .. Length( parameters ) ]};
vars := SplitString( vars, "," );
else
ErrorWithPos( Concatenation( "Wrong InsertChunk parameters: ", parameters ) );
Copy link
Member

@fingolfin fingolfin Jan 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That error text is no very helpful. What is wrong?

fi;
inserted_dummy := DocumentationDummyInstance( tree, DocumentationDummy( tree, title ) );
inserted_dummy!.variable_values := vars;
Copy link
Member

@fingolfin fingolfin Jan 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd feel more natural to me pass vars to DocumentationDummyInstance (resp. to InsertChunk in the bigger change I suggest)

Add( current_item, inserted_dummy );
end,
@InsertSystem := ~.@InsertChunk,
@BeginChunk := function()
local parameters, title, vars, current_var;
if IsBound( current_item ) then
Add( context_stack, current_item );
fi;
current_item := DocumentationDummy( tree, current_command[ 2 ] );
parameters := SplitString( current_command[ 2 ], " " );
if Length( parameters ) = 1 then
vars := [ ];
title := parameters[ 1 ];
elif Length( parameters ) = 2 then
vars := parameters[ 2 ];
vars := SplitString( vars, "," );
Copy link
Member

@fingolfin fingolfin Jan 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add a check like if not ForAll(vars, IsValidIdentifier) then Error(...); fi; to prevent users from doing weird things (like using a param name containing %). While it might not matter much right now, it will make future extensions easier.

Thinking about it, the chunk name should also be restricted this way.

Copy link
Author

@sebasguts sebasguts Jan 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can restrict the var names to be alphanumeric, probably with _ in it. Will do that.

title := parameters[ 1 ];
else
ErrorWithPos( Concatenation( "Wrong chunk parameters: ", current_command[ 2 ] ) );
fi;
if not ForAll( title, i -> i in AUTODOC_IdentifierLetters ) then
ErrorWithPos( Concatenation( "Wrong character in chunk title: ", title ) );
Copy link
Member

@fingolfin fingolfin Jan 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That error message is not very helpful. Which character is wrong? And what does that mean? Perhaps "Invalid chunk name TITLE (chunk names must be identifiers)" -- the user can lookup in the manual what an "identifier" is. Or else explicitly say it, e.g., "(chunk names must only contain letters, digits and underscores)"

fi;
for current_var in vars do
if not ForAll( current_var, i -> i in AUTODOC_IdentifierLetters ) then
ErrorWithPos( Concatenation( "Wrong character in chunk parameter: ", current_var ) );
Copy link
Member

@fingolfin fingolfin Jan 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar as above.

fi;
od;
current_item := DocumentationDummy( tree, title );
current_item!.variable_names := vars;
end,
@Chunk := ~.@BeginChunk,
@System := ~.@BeginChunk,
Expand Down