Added parametrized chunks #147

sebasguts · 2018-01-02T14:54:07Z

Added the possibility to parametrize chunks with variables and define their values when the chunks are included.

fingolfin · 2018-01-04T10:51:15Z

doc/Comments.xml

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

+In the text, you can use <A>%param1</A> placeholders, which will then be overwritten


I would suggest to use %param1% instead of %param1, otherwise, it becomes hard to impossible to insert a placeholder not followed by a space.

Alternatively, switch to asymmetric delimiters which then also might be used in future extensions. E.g. use {param1}, and then later one can (if desired) add more complex substitution rules.

I switched to %{param1}.

fingolfin · 2018-01-04T10:51:53Z

doc/Comments.xml

+#! @InsertChunk MyChunk world
 ## The text "Hello, world." is inserted right before this.
+## The text "Hello, Max." is inserted right behind this comment
+#! @InsertChunk MyChunk Max


Please change Max to something more universal, e.g. universe or user, or ...

fingolfin · 2018-01-04T10:53:09Z

gap/DocumentationTree.gd

 DeclareOperation( "DocumentationExample", [ IsTreeForDocumentation ] );
 DeclareOperation( "DocumentationDummy", [ IsTreeForDocumentation, IsString, IsList ] );
 DeclareOperation( "DocumentationDummy", [ IsTreeForDocumentation, IsString ] );
+DeclareOperation( "DocumentationDummyInstance", [ IsTreeForDocumentation, IsTreeForDocumentationNode ] );


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

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.

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).

fingolfin · 2018-01-04T10:57:16Z

gap/Parser.gi

+                title := parameters[ 1 ];
+            elif Length( parameters ) = 2 then
+                vars := parameters[ 2 ];
+                vars := SplitString( vars, "," );


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.

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

fingolfin · 2018-01-04T10:58:46Z

doc/Comments.xml


+In the text, you can use <A>%param1</A> placeholders, which will then be overwritten
+by the <A>param1_value</A> parameters when the text is inserted. Different parameters
+may not contain spaces and must be separated by a comma. Different values may contain


What is meant with "Different parameters"? Do you mean the parameter names?

I'd in fact restrict them to valid identifiers, see below.

Also, "Different parameters" sounds weird to me. What are "Equal parameters"? Same for "Different values". The more I think about it, I'd completely rewrite this paragraph. Here is a suggestion:

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 param, every occurrence of the string {param} is substituted by the corresponding parameter value.

fingolfin · 2018-01-04T11:01:42Z

doc/Comments.xml

+by the <A>param1_value</A> parameters when the text is inserted. Different parameters
+may not contain spaces and must be separated by a comma. Different values may contain
+spaces and must be separated by a comma. There must be a value for each parameter. Please
+note that this is not checked by &AutoDoc;.


I decided not to do this. I can only check this at the point of writing out the xml, as the insertion might be read before the actual chunk. Then I could not point to the place where it is wrong. I can still provide an error message, but it might not contain all information people want. Will insert it anyway.

fingolfin · 2018-01-04T16:13:36Z

gap/DocumentationTree.gd

 ######################################

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


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

fingolfin · 2018-01-04T16:15:28Z

gap/DocumentationTree.gd

 DeclareOperation( "DocumentationExample", [ IsTreeForDocumentation ] );
 DeclareOperation( "DocumentationDummy", [ IsTreeForDocumentation, IsString, IsList ] );
 DeclareOperation( "DocumentationDummy", [ IsTreeForDocumentation, IsString ] );
+DeclareOperation( "DocumentationDummyInstance", [ IsTreeForDocumentation, IsTreeForDocumentationNode ] );


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).

fingolfin · 2018-01-04T16:16:08Z

gap/DocumentationTree.gi

+                       IsTreeForDocumentationNodeRep,
+                       [ ] );
+
+BindGlobal( "TheTypeOfDocumentationTreeDummyInstanceNodes",


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

fingolfin · 2018-01-04T16:33:35Z

gap/Parser.gi

+                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 ) );


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)"

fingolfin · 2018-01-04T16:34:02Z

gap/Parser.gi

+            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 ) );


Similar as above.

fingolfin · 2018-01-04T16:37:51Z

gap/Parser.gi

+                vars := parameters{[ position_first_space + 1 .. Length( parameters )  ]};
+                vars := SplitString( vars, "," );
+            else
+                ErrorWithPos( Concatenation( "Wrong InsertChunk parameters: ", parameters ) );


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

fingolfin · 2018-01-04T16:39:08Z

gap/DocumentationTree.gi

+    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" );


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?

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...)

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

fingolfin · 2018-01-04T17:06:54Z

gap/Parser.gi

+                ErrorWithPos( Concatenation( "Wrong InsertChunk parameters: ", parameters ) );
+            fi;
+            inserted_dummy := DocumentationDummyInstance( tree, DocumentationDummy( tree, title ) );
+            inserted_dummy!.variable_values := vars;


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

fingolfin · 2018-01-13T18:03:40Z

@sebasguts just to make sure there is no misunderstanding: I am waiting for revisions resp. responses by you, so I am not otherwise thinking about this PR anymore.

fingolfin · 2018-08-28T11:29:25Z

@sebasguts this needs to be rebased, and comments be addressed

fingolfin · 2019-05-22T07:50:38Z

@sebasguts unless you have plans to resurrect this PR, let's close it. I am not terribly fond of this feature anyway ;-). But of course if you or somebody else (@mohamed-barakat ) really, really needs this, we can get something like this in; but then the commit message should at least contain some motivation as to why this is useful.

sebasguts · 2019-05-22T10:14:37Z

Actually, I would like to keep it for now. Will give more comments later.

fingolfin · 2019-05-22T13:23:30Z

Sure, no hurry. It's not hurting us to have it open either :-)

sebasguts mentioned this pull request Jan 3, 2018

Update CHANGES file #148

Closed

fingolfin requested changes Jan 4, 2018

View reviewed changes

Added parametrized chunks

b394409

sebasguts force-pushed the parametrized_chunks branch from 0932998 to b394409 Compare January 4, 2018 13:34

fingolfin reviewed Jan 4, 2018

View reviewed changes

Added parametrized chunks #147

Are you sure you want to change the base?

Added parametrized chunks #147

Uh oh!

Conversation

sebasguts commented Jan 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fingolfin commented Jan 13, 2018

Uh oh!

fingolfin commented Aug 28, 2018

Uh oh!

fingolfin commented May 22, 2019

Uh oh!

sebasguts commented May 22, 2019

Uh oh!

fingolfin commented May 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants