Add BannerFunction to PackageInfo.g

[fingolfin]

This implements some in the idea described in issue #2568, by adding two new keys to the package info record (their names are of course still open for discussion):

• BannerExtra can set to a string, or to a function returning a string; the string will be printed as part of the the default banner, as produced by DefaultPackageBannerString. This can be used to print some additional information in the banner, e.g. extra acknowledgements; or dynamic info about build configuration for a package with kernel extension.

• BannerFunction can be set to a function which takes the info record as argument, and returns a string which is used just like BannerString. This way, one can print a custom banner string with dynamic data, such as information about the build configuration for a package with kernel extension.

One alternative suggestion was to just allow setting BannerString to a function. What I don't like about that is that such a package then cannot be loaded by older GAP versions anymore. While with the approach in this PR, a package using either of the two new keys would still be useable in older GAP versions.

Still missing is documentation for these new keys -- but I am not going to write that unless I know we'll (very likely) merge this.

UPDATE: I removed BannerExtra

Closes #2568

[ThomasBreuer]

I do not see why BannerExtra is helpful.
Adding extra information just at the end of the default banner (which would have to be documented) is probably not what one wants.
Note that one can easily call DefaultPackageBannerString from the BannerFunction function,
and then modify the returned string as needed.

I would prefer the simpler alternative to admit a function as BannerString.
Note that the feature is anyhow new, and if one wants to use it then it is natural to require, in the package's dependencies, a GAP version that supports it.

[laurentbartholdi]

I agree that BannerExtra seems redundant. I like the idea of passing the PackageInfo record, which is otherwise cumbersome to recover. For me, BannerFunction is perfect.

[codecov]

Codecov Report

Merging #2598 into master will increase coverage by <.01%.
The diff coverage is 75%.

@@            Coverage Diff             @@
##           master    #2598      +/-   ##
==========================================
+ Coverage    74.8%    74.8%   +<.01%     
==========================================
  Files         479      479              
  Lines      242251   242254       +3     
==========================================
+ Hits       181206   181211       +5     
+ Misses      61045    61043       -2

Impacted Files	Coverage Δ
lib/package.gi	69.86% <75%> (-0.01%)	⬇️
hpcgap/lib/hpc/stdtasks.g	64.19% <0%> (+0.76%)	⬆️

[laurentbartholdi]

I really think that the extra functionality in BannerExtra is best left out. For users who already have BannerExtra set, they can just add

BannerFunction := info->Concatenation(DefaultPackageBannerString(info),info.BannerExtra);

[fingolfin]

Adding data to the default package string is not quite that trivial, and the example @laurentbartholdi gives does not quite work the way I'd expect it to work, because it adds the extra data after the trailing separator line. So, to insert data, one has to split the string into lines, then insert additional lines before the final line, and re-assemble the line (or do something equivalent, e.g. just find the start of the last line, and insert before that.

While this is not very hard, it's cumbersome.

The reason why I thought BannerExtra might be useful is that I expect most people would just want to add info to the default banner, but not change anything else about it. So having a convenient way to do that is, well, convenient. That said, I myself always use the default banner, so I don't mind dropping this again very much.

As to BannerString vs. BannerFunction: For it to have any effect, you indeed need to run a new GAP anyhow. But if you are author of a package that otherwise works great with GAP 4.8, it seems undesirable to me to change it to require GAP 4.10, just to take advantage of a different way of printing banners. By using BannerFunction, your package can still be used in GAP 4.8 (with a default banner).

And of course all of this would have to be documented -- as I stated, I did not yet write documentation because the future of these is unclear (and the discussion here proves that point quite well ;-)

[fingolfin]

@laurentbartholdi your example rather would look like this:

BannerFunction := function(info)
  local l;
  l:=SplitString(DefaultPackageBannerString(info), "\n");
  Add(l, info.BannerExtra, Length(l));
  return JoinStringsWithSeparator(l,"\n");
end;

Anyway, I certainly don't mind not adding BannerExtra if people don't think its useful shrug

[laurentbartholdi]

Thanks for the correction. Indeed, it's not the 1-liner I thought of. I'm definitely not religious about either, but I have the feeling that BannerFunction is enough, because most packages won't need any extra anyways, so we're only talking about a handful of packages. This makes me vote slightly in favour of the minimal amount of bloat. In all cases, I strongly support addition of either BannerFunction or BannerFunction+BannerExtra.

…

On Tue, Jul 3, 2018 at 12:39 PM Max Horn ***@***.***> wrote: @laurentbartholdi <https://github.com/laurentbartholdi> your example rather would look like this: BannerFunction := function(info) local l; l:=SplitString(DefaultPackageBannerString(info), "\n"); Add(l, info.BannerExtra, Length(l)); return JoinStringsWithSeparator(l,"\n");end; Anyway, I certainly don't mind not adding BannerExtra if people don't think its useful *shrug* — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2598 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACIIUL9a00NuTfVhDUUqwUmAvUIeMOsAks5uC0nFgaJpZM4U_M4t> .

-- Laurent Bartholdi laurent.bartholdi<at>gmail<dot>com École Normale Supérieure: Téléphone +33 1 44322060 Bureau T5, Toits du DMA, 45 Rue d'Ulm, 75005 Paris G.-A. Universität zu Göttingen: Phone: +49 551 39 7826 Office 202, Mathematisches Institut, Bunsenstrasse 3-5, D-37073 Göttingen.

[frankluebeck]

How many packages are there who potentially want to use these new features? Not many, I guess.

Therefore I would say that ultimate convenience or compatibility with old GAP versions are not a practical issue here.

I would just extend the definition of 'BannerString' and allow to set this to a function that gets the package info record as argument and must return a string (or to a string as before).

[olexandr-konovalov]

As soon as a package will start to use the extended definition of the BannerString and will have a function there, its development version and its new releases will not be usable with GAP 4.9 release series, and all updates there will be frozen until GAP 4.10. So I do see the point in compatibility achieved by BannerFunction.

Second, as @fingolfin explained, BannerExtra allows to make a small modification of the banner when one does not want to recreate it completely. That seems quite useful to me exactly by his reasoning "I expect most people would just want to add info to the default banner, but not change anything else about it.". In this case, using a functions seems like an unnecessary overhead demanded from package authors.

[olexandr-konovalov]

P.S. One more thing: the package update mechanism runs the officially released GAP version. So ValidatePackageInfo there will not validate any new release of the package which will have a function in BannerString because it is not a string. That's why it's much simpler to introduce a new field rather then to change an existing one.

[fingolfin]

I have no removed BannerExtra. I still think having BannerFunction separate from BannerString is useful, as explained, even though right now I only know of two packages which might use this (JuliaInterface and float)

[olexandr-konovalov]

I am happy with the current form now (i.e. with just one new BannerFunction component. But where this should be documented? Perhaps in the Example package, in its PackageInfo.g file. You can provide an example, comment it out, and also write in the comment that this will be available since GAP 4.10.

[fingolfin]

@laurentbartholdi is this OK for you now? then please remove your "request for changes" flag, or even approve it. If not, then please state what you'd like to see changed. Thanks!

@alex-konovalov I think documenting this in doc/ref/gappkg.xml is enough, no need to put this obscure thing into the example package. But I'd like to see a major overhaul of doc/ref/gappkg.xml happening, to make it easier to find each possible record name possible in a package info record.

[olexandr-konovalov]

@fingolfin doc/ref/gappkg.xml refers to the template in Example, and does not describe it in great details otherwise. Unless you really want to spend time now and document all of PackageInfo.g there, doing that in the Example package for now is what I'd strongly advise.

[olexandr-konovalov]

@fingolfin I'd definitely want this to be documented before merging, and unless you have time for a a major overhaul of doc/ref/gappkg.xml now (what I really doubt), could you please just document that (and provide a commented out example) in comments in https://github.com/gap-packages/example/blob/master/PackageInfo.g, and then we could merge this?

[olexandr-konovalov]

@fingolfin Alternatively, we can merge this first, and I will remember to add text to https://github.com/gap-packages/example/blob/master/PackageInfo.g

[fingolfin]

I agree that documenting this first in the example package is the pragmatic solution for now; an overhaul of gappkg, while IMHO desirable, still might never happen.

Here is some text for the example package:

##  *Optional*: if you need a custom BannerString but would like to include
##  information in it that is only available once your package is being loaded
##  (i.e., which is computed in your init.g file, such as the presence and
##  versions of external software your package depends on), then you can
##  use a BannerFunction instead. The difference is that the BannerString is
##  usually computed when GAP starts, i.e., long before your init.g is run.
##  While the BannerFunction is called right before the banner is to be
##  displayed, which is after your init.g has been executed.
##
# BannerFunction := function(info)
#       local l;
#       # modify the default banner string, and insert something before
#       # its last line (which is a separator string)
#       l:=SplitString(DefaultPackageBannerString(info), "\n");
#       Add(l, " ...  some extra information ... ", Length(l));
#       return JoinStringsWithSeparator(l,"\n");
#     end,

[olexandr-konovalov]

Thanks for the text for PackageInfo.g - happy to merge this now.

[olexandr-konovalov]

@fingolfin great. Text added to Example package's PackageInfo.g in gap-packages/example@aae6a93