Skip to content

Add @GroupTitle and @GroupInitialArguments commands #172

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

Closed
gwhitney wants to merge 1 commit into from
Closed

Add @GroupTitle and @GroupInitialArguments commands #172

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
40 changes: 36 additions & 4 deletions doc/Comments.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,25 @@ DeclareOperation( "GroupedOperation",
]]></Listing>
</Subsection>

<Subsection Label="@GroupTitle">
<Index Key="@GroupTitle"><C>@GroupTitle</C></Index>
<Heading>@GroupTitle <A>title</A></Heading>
Sets the subsection heading for the current group to <A>title</A>. In the
absence of any <C>@GroupTitle</C> command, the heading will be the name of the
first entry in the group. See <Ref Sect="Groups" /> for more information.
Copy link
Copy Markdown
Member

@fingolfin fingolfin Sep 14, 2018

Choose a reason for hiding this comment

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

@GroupTitle seems like a sensible addition to me.

</Subsection>

<Subsection Label="@GroupInitialArguments">
<Index Key="@GroupInitialArgumentss"><C>@GroupInitialArguments</C></Index>
Copy link
Copy Markdown
Member

@fingolfin fingolfin Sep 14, 2018

Choose a reason for hiding this comment

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

Typo: GroupInitialArgumentss with double s

<Heading>@GroupInitialArguments <A>args</A></Heading>
Sets the common initial arguments for the entries in the current group to be
<A>args</A>. The argument list for any function subsequently added to the
group without any args specified will be <A>args</A>, and <A>args</A> will be
prepended to any arguments specified for a particular function. The setting is
cleared by an <C>@EndGroup</C> command. See <Ref Sect="Groups" /> for more
information.
Copy link
Copy Markdown
Member

@fingolfin fingolfin Sep 14, 2018

Choose a reason for hiding this comment

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

I don't like @GroupInitialArguments at all. It makes information non-local: with it, when reading a function's AutoDoc comment, I must be aware of whether it is inside a group, and whether a @GroupInitialArguments was specified or not.

This does not seem to justify the (IMHO very minor, if at all) "savings" provided by it. It's not that difficult to repeat the string order, two or three times (it won't be more in general).

</Subsection>

<Subsection Label="@Level">
<Index Key="@Level"><C>@Level</C></Index>
<Heading>@Level <A>lvl</A></Heading>
Expand Down Expand Up @@ -556,8 +575,9 @@ a function, operation, etc., it is possible to group them into suitable chunks.
This can be particularly useful if there are several definitions of an operation
with several different argument types, all doing more or less the same to the arguments.
Then their manual items can be grouped, sharing the same description and return type information.
Note that it is currently not possible to give a header to the Group in the manual,
but the generated ManItem heading of the first entry will be used.
You can give a heading to the Group in the manual with the <C>@GroupTitle</C>
Copy link
Copy Markdown
Member

@fingolfin fingolfin Sep 14, 2018

Choose a reason for hiding this comment

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

Group -> group (yes, this was misspelled before.

command; if that is not supplied, then the generated ManItem heading of the
Copy link
Copy Markdown
Member

@fingolfin fingolfin Sep 14, 2018

Choose a reason for hiding this comment

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

ManItem is also bad here, and should not be used -- but of course it was here before, so I do not expect you to fix it (rather, I expect me or @sebasguts to fix it)

Copy link
Copy Markdown

@sebasguts sebasguts Sep 15, 2018

Choose a reason for hiding this comment

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

Why do you think this is bad? ManItem is a special kind of subsection in GAPDoc, which does not need to have a heading, but will create one. What would you suggest?

Copy link
Copy Markdown
Member

@fingolfin fingolfin Sep 15, 2018

Choose a reason for hiding this comment

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

First off, it should at the very least be typeset with <C> or so -- better would be a Ref to the relevant GAPDoc manual entry, though.

But really, why is a technical term from GAPDoc used to explain things here? In order to understand this, now one needs to be familiar with both AutoDoc and GAPDoc, and how the conversion from one to the other works exactly.

It would be much better if the AutoDoc explanation was self-contained. Perhaps in the future, we do not just output GAPDoc, but also some other format?

At the same time, it can be useful to document how certain AutoDoc constructs translate to GAPDoc. But IMHO this should be separate (say, in a separate paragraph). And then for more (ideally all) AutoDoc, not just a random (?) aubset

Copy link
Copy Markdown

@sebasguts sebasguts Sep 15, 2018

Choose a reason for hiding this comment

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

Yep, you are right, it is probably bad to refer to GAPDoc internals (probably out of laziness here). Anyway, as you stated, I should probably fix this myself, so this should not block this PR.

first entry will be used as the heading.
<P/>

Note that group names are globally unique throughout the whole manual.
Expand All @@ -570,16 +590,27 @@ same group name, in different places, and these all will refer to the same group
Moreover, this means that you can add items to a group via the <C>@Group</C> command
in the &AutoDoc; comment of an arbitrary declaration, at any time.

<P/>
Since often the functions in a Group share common initial arguments, you can
set a list of common arguments to be prepended to all declarations within a
given <C>@BeginGroup/@EndGroup</C> pair with the <C>@GroupInitialArguments</C>
command.

<P/>

The following code
<Listing><![CDATA[
#! @BeginGroup Group1
#! @GroupTitle A family of operations
#! @GroupInitialArguments order

#! @Description
#! First sentence.
DeclareOperation( "FirstOperation", [ IsInt ] );

#! @Description
#! Second sentence.
#! @Arguments ambient_group
DeclareOperation( "SecondOperation", [ IsInt, IsGroup ] );

#! @EndGroup
Expand All @@ -595,8 +626,9 @@ KeyDependentOperation( "ThirdOperation", IsGroup, IsInt, "prime );
produces the following:

<ManSection Label="Group1">
<Oper Arg="arg" Name="FirstOperation" Label="for IsInt"/>
<Oper Arg="arg1,arg2" Name="SecondOperation" Label="for IsInt, IsGroup"/>
<Heading>A family of operations</Heading>
<Oper Arg="order" Name="FirstOperation" Label="for IsInt"/>
<Oper Arg="order,ambient_group" Name="SecondOperation" Label="for IsInt, IsGroup"/>
<Oper Arg="arg1,arg2" Name="ThirdOperation" Label="for IsGroup, IsInt"/>
<Returns></Returns>
<Description>
Expand Down
7 changes: 6 additions & 1 deletion gap/DocumentationTree.gi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -623,10 +623,15 @@ end );
##
InstallMethod( WriteDocumentation, [ IsTreeForDocumentationNodeForGroupRep, IsStream ],
function( node, filestream )
local head;
Copy link
Copy Markdown
Member

@fingolfin fingolfin Mar 28, 2019

Choose a reason for hiding this comment

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

Suggested change
local head;
local heading;

if node!.level > ValueOption( "level_value" ) then
return;
fi;
AutoDoc_WriteDocEntry( filestream, node!.content );
head := fail;
Copy link
Copy Markdown
Member

@fingolfin fingolfin Mar 28, 2019

Choose a reason for hiding this comment

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

Suggested change
head := fail;
heading := fail;

if IsBound( node!.title_string ) then
head := node!.title_string;
Copy link
Copy Markdown
Member

@fingolfin fingolfin Mar 28, 2019

Choose a reason for hiding this comment

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

Suggested change
head := node!.title_string;
heading := node!.title_string;

fi;
AutoDoc_WriteDocEntry( filestream, node!.content, head );
Copy link
Copy Markdown
Member

@fingolfin fingolfin Sep 14, 2018

Choose a reason for hiding this comment

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

Please rename head to heading.

Copy link
Copy Markdown
Member

@fingolfin fingolfin Mar 28, 2019

Choose a reason for hiding this comment

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

Suggested change
AutoDoc_WriteDocEntry( filestream, node!.content, head );
AutoDoc_WriteDocEntry( filestream, node!.content, heading );

end );

##
Expand Down
54 changes: 50 additions & 4 deletions gap/Parser.gi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,11 @@ InstallGlobalFunction( AutoDoc_Type_Of_Item,
entries := [ "Func", "global_functions" ];
has_filters := "No";
if not IsBound( item_rec!.arguments ) then
item_rec!.arguments := "arg";
if IsBound( item_rec!.initial_args ) then
item_rec!.arguments := item_rec!.initial_args;
else
item_rec!.arguments := "arg";
fi;
fi;
elif PositionSublist( type, "DeclareGlobalVariable" ) <> fail then
entries := [ "Var", "global_variables" ];
Expand Down Expand Up @@ -116,7 +120,7 @@ InstallGlobalFunction( AutoDoc_Parser_ReadFiles,
function( filename_list, tree, default_chapter_data )
local current_item, flush_and_recover, chapter_info, current_string_list,
Scan_for_Declaration_part, flush_and_prepare_for_item, current_line, filestream,
level_scope, scope_group, read_example, command_function_record, autodoc_read_line,
level_scope, scope_group, scope_initial_args, read_example, command_function_record, autodoc_read_line,
current_command, was_declaration, filename, system_scope, groupnumber, chunk_list, rest_of_file_skipped,
context_stack, new_man_item, add_man_item, Reset, read_code, title_item, title_item_list, plain_text_mode,
current_line_unedited,
Expand Down Expand Up @@ -159,6 +163,9 @@ InstallGlobalFunction( AutoDoc_Parser_ReadFiles,
if IsBound( scope_group ) then
SetGroupName( man_item, scope_group );
fi;
if IsBound( scope_initial_args ) then
man_item!.initial_args := scope_initial_args;
fi;
man_item!.chapter_info := ShallowCopy( chapter_info );
man_item!.tester_names := fail;
return man_item;
Expand Down Expand Up @@ -267,7 +274,10 @@ InstallGlobalFunction( AutoDoc_Parser_ReadFiles,
##Adjust arguments
if not IsBound( current_item!.arguments ) then
if IsInt( has_filters ) then
if has_filters = 1 then
if IsBound( current_item!.initial_args ) then
current_item!.arguments :=
current_item!.initial_args;
elif has_filters = 1 then
current_item!.arguments := "arg";
else
current_item!.arguments := JoinStringsWithSeparator( List( [ 1 .. has_filters ], i -> Concatenation( "arg", String( i ) ) ), "," );
Expand Down Expand Up @@ -561,8 +571,14 @@ InstallGlobalFunction( AutoDoc_Parser_ReadFiles,
fi;
scope_group := ReplacedString( current_command[ 2 ], " ", "_" );
end,
@GroupInitialArguments := function()
scope_initial_args := current_command[ 2 ];
current_item := new_man_item();
current_item!.initial_args := current_command[ 2 ];
end,
@EndGroup := function()
Unbind( scope_group );
Unbind( scope_initial_args );
end,
@Description := function()
current_item := new_man_item();
Expand All @@ -580,8 +596,16 @@ InstallGlobalFunction( AutoDoc_Parser_ReadFiles,
fi;
end,
@Arguments := function()
local composed_arguments;
if IsBound( scope_initial_args ) and
Length( scope_initial_args ) > 0 then
composed_arguments :=
Concatenation( scope_initial_args, ", ", current_command[ 2 ] );
else
composed_arguments := current_command[ 2 ];
fi;
current_item := new_man_item();
current_item!.arguments := current_command[ 2 ];
current_item!.arguments := composed_arguments;
end,
@Label := function()
current_item := new_man_item();
Expand All @@ -593,6 +617,28 @@ InstallGlobalFunction( AutoDoc_Parser_ReadFiles,
group_name := ReplacedString( current_command[ 2 ], " ", "_" );
SetGroupName( current_item, group_name );
end,
@GroupTitle := function()
local group_name, chap_info, group_obj;
current_item := new_man_item();
if not HasGroupName( current_item ) then
ErrorWithPos( "found @GroupTitle with no Group set" );
fi;
group_name := GroupName( current_item );
chap_info := fail;
if HasChapterInfo( current_item ) then
chap_info := ChapterInfo( current_item );
elif IsBound( current_item!.chapter_info ) then
chap_info := current_item!.chapter_info;
fi;
if (chap_info = fail or chap_info = [ ]) then
Copy link
Copy Markdown
Member

@fingolfin fingolfin Mar 28, 2019

Choose a reason for hiding this comment

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

Suggested change
if (chap_info = fail or chap_info = [ ]) then
if chap_info = fail or chap_info = [ ] then

chap_info := chapter_info;
fi;
if Length( chap_info ) <> 2 then
ErrorWithPos( "can only set @GroupTitle within a Chapter and Section.");
fi;
group_obj := DocumentationGroup( tree, group_name, chap_info );
group_obj!.title_string := current_command[ 2 ];
end,
@ChapterInfo := function()
local current_chapter_info;
current_item := new_man_item();
Expand Down
12 changes: 10 additions & 2 deletions gap/ToolFunctions.gi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ end );

##
InstallGlobalFunction( AutoDoc_WriteDocEntry,
function( filestream, list_of_records )
function( filestream, list_of_records, maybe_heading... )
Copy link
Copy Markdown
Member

@fingolfin fingolfin Sep 14, 2018

Choose a reason for hiding this comment

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

I don't like having maybe_heading an optional argument. Just change maybe_heading... to heading.

Copy link
Copy Markdown
Member

@fingolfin fingolfin Mar 28, 2019

Choose a reason for hiding this comment

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

To be clearer: I suggest to change this to this:

Suggested change
function( filestream, list_of_records, maybe_heading... )
function( filestream, list_of_records, heading )

Then callers should either pass a string for heading, or something else, like fail, to indicate no heading is desired. That'll make the code a bit simpler, I'd say.

local return_value, description, current_description, labels, i;

# look for a good return value (it should be the same everywhere)
Expand Down Expand Up @@ -78,7 +78,15 @@ InstallGlobalFunction( AutoDoc_WriteDocEntry,
od;
AppendTo( filestream, ">\n" );

# Function heades
# Next possibly the heading for the entry
if Length( maybe_heading ) > 0 then
maybe_heading := maybe_heading[ 1 ];
if IsString( maybe_heading ) then
AppendTo( filestream, "<Heading>", maybe_heading, "</Heading>\n" );
fi;
fi;
Copy link
Copy Markdown
Member

@fingolfin fingolfin Sep 14, 2018

Choose a reason for hiding this comment

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

With the change to heading, this could be replaced by

if IsString( heading ) then
  AppendTo( filestream, "<Heading>", heading, "</Heading>\n" );
fi;


# Function headers
for i in list_of_records do
AppendTo( filestream, " <", i!.item_type, " " );
if i!.arguments <> fail and i!.item_type <> "Var" then
Expand Down