-
Notifications
You must be signed in to change notification settings - Fork 160
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.
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
Print issue tracker, maintainers and contributors in package banners #3215
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Of course, the banners in tst/testinstall/package.tst
must be adjusted to this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I am interested in the details of a particular package,
for example because I want to report a bug,
it is certainly useful to provide all this technical information,
thus there should be a function for that.
However, as far as I see, currently the package banner is mainly shown
when a package gets loaded,
and I would rather prefer to reduce the number of lines shown in this situation;
printing the package name and the version number would be sufficient
from my point of view.
@fingolfin @ThomasBreuer I'ld suggest that we use the way package banners are printed like in this PR as the default. They provide useful information to people that are not that familiar with GAP. OTOH there could be a |
In the Jupyter (and any similar) interfaces, it would be nice to "fold" this information away by default, so that what printed was a link you could click on to expand the details in some way. While that obviously goes beyond the scope of this PR, are there any considerations in the way it is implemented that might make that easier in the future? @markuspf? |
If available, print the issue tracker URL as part of the default package banner. Also, if the list of package maintainers is non-empty and different from the list of package authors, always print it. Both changes are meant to help users to identify whom to contact about issues. Finally, also list pure contributors (neither author nor maintainer) in default package banner.
6b8811d
to
8bb3398
Compare
Codecov Report
@@ Coverage Diff @@
## master #3215 +/- ##
==========================================
- Coverage 84.76% 75.78% -8.98%
==========================================
Files 687 632 -55
Lines 335864 281610 -54254
==========================================
- Hits 284690 213420 -71270
- Misses 51174 68190 +17016
|
@frankluebeck I have now updated and extended the banner tests. @ThomasBreuer @ssiccha I agree that an option / user preference to get compact banners would be useful (of course it'd only affect packages using the default banner -- and this PR is hopefully going to allow a few more packages to switch to the default banner). I'd be happy to review such a PR! |
@ssiccha I disagree. I like the idea of @stevelinton to show the long version of a package banner on demand. |
@ThomasBreuer Ah, one could do something like print the compact banner per default and have a line like Would that step on any package authors, maintainers, or contributors' toes since then their names won't be printed when loading a package? |
@ssiccha I think this is too radical reduction which is on the other side of the scale, opposite to @fingolfin's suggestion. To make it shorter, perhaps we can print people's names but not their homepage URLs? While shortening, we should remember that |
I also think this might make input too long. As a practical example, at the moment loading digraphs is 21 lines -- quite a lot of screen but at least I can still see what I was typing. With this PR it becomes 37 lines, scrolling whatever I was doing way off the top of the terminal. (Maybe) controversial suggestion -- could we output a much shorter banner (just title/version?) for packages which are required by the package the user actually wanted to load -- not sure how easy that would be to do. I really don't like the idea of a configuration option, as such things are hard to discover. |
@ChrisJefferson just title/version results in people sometime asking questions all over the internet, but not in the right place, or googling and discovering outdated pages hosted elsewhere, but not latest package pages. That's why I said that we should remember that Many lines are because one contributor occupies one line (well done, Digraphs!). If one can list authors without URLs and combine several names in one line, that will make it much shorter! |
I will cope with displaying information about packages which a user explicitly loaded. DIsplaying it for required packages doesn't sound useful -- If a user of semigroups finds a bug, how are they supposed to know if they should report it at the bug tracker of semigroups, digraphs, GRAPE, genss, orb, IO or GAP? |
I think this output is probably too long. When some packages from the homalg project are loaded, they might as well load 20 packages with them (I know, this is a different problem). Adding more information to the banners makes it harder to find information in the output for those who want to find, say, loaded package versions. Other people might just be overwhelmed by all the information and might never look at it either. Furthermore, I think @alex-konovalov has a point that when an error occurs, it is unclear to which issue tracker a bug report has to be send, as there might be several to choose from. @stevelinton : In Jupyter folding could be done by returning the banner strings as HTML, probably using the |
I think that we have two issues. Could we merge the current pull request, |
If available, print the issue tracker URL as part of the default package banner. Also, if the list of package maintainers is non-empty and different from the list of package authors, always print it. Both changes are meant to help users to identify whom to contact about issues.
Finally, any people in the person record who are neither author nor maintainer are listed under "with contributions by". This matches what GitHubPagesForGAP does, and how various packages seem to use these fields. It means that we now really list all people in the persons record at least once in the default package banner.
Some examples:
All persons are both author and maintainer: no change
List of authors and maintainers differ
Packages "with contributions by"
Some packages have "contributors", e.g. in Homalge (ping to @mohamed-barakat). I think this change would also allow
semigroups
,recog
andxmod
to switch to the default package banner.