Add user preference ShortBanners (if enabled package banners are reduced to just name, version and description of a package)
#5407
Conversation
zickgraf
force-pushed
the
short_banners
branch
2 times, most recently
from
585778a to
ecf0d47
Compare
|
The CI is failing due to the removed duplicate space, but I will wait for feedback before updating the tests. |
| @@ -1124,7 +1137,7 @@ InstallGlobalFunction( DefaultPackageBannerString, function( inforec ) | |||
| # Add package name and version number. | |||
| if IsBound( inforec.PackageName ) and IsBound( inforec.Version ) then | |||
| Append( str, Concatenation( | |||
| "Loading ", inforec.PackageName, " ", inforec.Version ) ); | |||
| "Loading ", inforec.PackageName, " ", inforec.Version ) ); | |||
There was a problem hiding this comment.
I am somewhat wary that this might break package tests. Although a quick look suggests that this is not the case, so perhaps all is good in that regard.
I can't think of a good reason for the extra space otherwise.
There was a problem hiding this comment.
I have now (hopefully) fixed the tests in GAP. Maybe we can merge and see how many packages are affected by this, if any at all? If the number is small I could open PRs, otherwise we could revert.
zickgraf
force-pushed
the
short_banners
branch
2 times, most recently
from
1aec53e to
4c50984
Compare
lib/package.gi
Outdated
| description:= [ | ||
| "If this option is set to <K>true</K>, only the first line of each \ | ||
| package banner (including the name, version and description of the package) \ | ||
| is printed for packages using the default banner." |
There was a problem hiding this comment.
OK, I've thought some more about this, and I wonder if it wouldn't be more useful if this option affected all packages. I.e., if it is on, then all packages just get a shortened banner printed when loaded. To implement this, instead of modifying DefaultPackageBannerString we could add a separate function, say ShortPackageBannerString. And then the code which prints the package banner makes a high-level decision what to do
| @@ -1429,7 +1455,8 @@ BindGlobal( "LoadPackage_ReadImplementationParts", | |||
| elif IsBound( info.BannerString ) then | |||
There was a problem hiding this comment.
So in this code, the first if could be changed to
if UserPreference( "ShortBanners" ) then
bannerstring := ShortPackageBannerString(info);
elif IsBound( info.BannerFunction ) then
...Thoughts?
There was a problem hiding this comment.
So custom banners would be complete ignored when ShortBanners is set to true? I just looked at some packages and, for example, curlInterface adds the versions of various libraries to the banner. I guess that this information is printed for debugging and in particular to have this information available in bug reports? If so, I think ignoring the custom banner would be suboptimal.
There was a problem hiding this comment.
A clean interface would maybe be that packages can specify additional information which gets included in the banner instead of letting BannerFunction patch DefaultPackageBannerString. And of course this additional information could then come in two variants, a long and a short version. But this would be beyond what I had planned for this PR :D
There was a problem hiding this comment.
The curl version can also be obtained in other ways.
And I'd try not to overthink this problem. Honestly, the whole quest for "short banners" is already extremely fringe. Most people will either not mind the long banner, or just turn banners off. Having a middle ground is not something I can see a lot of people having much interest in (I might be totally wrong, of course)
There was a problem hiding this comment.
The curl version can also be obtained in other ways.
And I'd try not to overthink this problem.
Since we aren't using custom banners I'm totally happy with just doing it this way! I have pushed a version which should achieve what you have described. It does not (yet) introduce a function ShortPackageBannerString but simply uses the switch I have already introduced for DefaultPackageBannerString. If you think a separate function is cleaner, I'm happy to implement this. Then the only question would be: Should DefaultPackageBannerString stay as it was originally (i.e. we would have some code duplication between ShortPackageBannerString and DefaultPackageBannerString) or should DefaultPackageBannerString call ShortPackageBannerString? I can see reasons for both approaches.
Honestly, the whole quest for "short banners" is already extremely fringe. Most people will either not mind the long banner, or just turn banners off. Having a middle ground is not something I can see a lot of people having much interest in
Interesting, for me this was a very obvious improvement. Maybe the homalg and CAP ecosystem just loads unusually large amount of packages? For example our package "FunctorCategories" currently loads 32 other packages. So with the usual banners there are simply screens full of messages. But turning of banners is also not a good solution because seeing which packages are loaded is useful for debugging, in particular in CI tests where I cannot easily rerun the tests with banners.
Disclaimer: I'm not trying to convince anyone that this is a much needed feature, I just want to elaborate on my personal motivation for this :D
|
Initiated PackageDistro CI run against this branch here: /~https://github.com/gap-system/PackageDistro/actions/runs/4494333014 |
If this option is set to <K>true</K>, package banners printed during loading will only show the name, version and description of a package.
|
Thanks @ChrisJefferson for the review! Maybe the PR could still make it into GAP 4.13? Or if there is not enough of an agreement that the option is useful, I would also be fine with just closing the PR. |
|
Thanks @fingolfin for the merge! |
fingolfin
changed the title
fingolfin
added
kind: new feature
topic: library
release notes: use title
fingolfin
changed the title
ShortBanners (if enabled package banners are reduced to just name, version and description of a package)
If this option is set to true, package banners printed during
loading will only show the name, version and description of a package.
Rationale: The package banners contain lots of information which one usually does not need during development (authors, homepage, link to the issue tracker) and which clutters the terminal, sometimes hiding information which one would actually need (like syntax warnings/errors).
The first commit simply removes a duplicate whitespace. If this whitespace is deliberate, I can of course add it back, although I would suggest to remove it at least for the short banners because I think it looks a bit out of place there.
Text for release notes
Add user preference "ShortBanners"
If this option is set to
true, package banners printed duringloading will only show the name, version and description of a package.