Skip to content
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

Merged
merged 1 commit into from
Jan 30, 2019

Conversation

fingolfin
Copy link
Member

@fingolfin fingolfin commented Jan 22, 2019

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

Loading  AutoDoc 2018.09.20 (Generate documentation from GAP source code)
by Sebastian Gutsche (http://wwwb.math.rwth-aachen.de/~gutsche/) and
   Max Horn (http://www.quendi.de/math).
Homepage: https://gap-packages.github.io/AutoDoc
Report issues at https://github.com/gap-packages/AutoDoc/issues

List of authors and maintainers differ

Loading  AutPGrp 1.10 (Computing the Automorphism Group of a p-Group)
by Bettina Eick (http://www.icm.tu-bs.de/~beick) and
   Eamonn O'Brien (http://www.math.auckland.ac.nz/~obrien).
maintained by:
   Bettina Eick (http://www.icm.tu-bs.de/~beick) and
   Max Horn (http://www.quendi.de/math).
Homepage: https://gap-packages.github.io/autpgrp/
Report issues at https://github.com/gap-packages/autpgrp/issues
Loading  AtlasRep 1.5.1 (An Atlas of Group Representations)
by Robert A. Wilson (http://www.maths.qmw.ac.uk/~raw),
   Richard A. Parker (richpark@gmx.co.uk),
   Simon Nickerson (http://nickerson.org.uk/groups),
   John N. Bray (http://www.maths.qmw.ac.uk/~jnb), and
   Thomas Breuer (http://www.math.rwth-aachen.de/~Thomas.Breuer).
maintained by:
   Thomas Breuer (http://www.math.rwth-aachen.de/~Thomas.Breuer).
Homepage: http://www.math.rwth-aachen.de/~Thomas.Breuer/atlasrep
Loading  IO 4.5.4 (Bindings for low level C library I/O routines)
by Max Neunhöffer (http://www-groups.mcs.st-and.ac.uk/~neunhoef).
maintained by:
   Max Horn (http://www.quendi.de/math).
Homepage: https://gap-packages.github.io/io
Report issues at https://github.com/gap-packages/io/issues

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 and xmod to switch to the default package banner.

Loading  HomalgToCAS 2018.06.15 (A window to the outer world)
by Thomas Bächler (http://wwwb.math.rwth-aachen.de/~thomas/),
   Mohamed Barakat (http://www.mathematik.uni-kl.de/~barakat/),
   Simon Görtzen (http://wwwb.math.rwth-aachen.de/~simon/),
   Sebastian Gutsche (http://wwwb.math.rwth-aachen.de/~gutsche/), and
   Vinay Wagh (http://www.iitg.ernet.in/vinay.wagh/).
with contributions by:
   Thomas Breuer (http://www.math.rwth-aachen.de/~Thomas.Breuer/) and
   Frank Lübeck (http://www.math.rwth-aachen.de/~Frank.Luebeck/).
maintained by:
   Mohamed Barakat (http://www.mathematik.uni-kl.de/~barakat/) and
   Simon Görtzen (http://wwwb.math.rwth-aachen.de/~simon/).
Homepage: http://homalg-project.github.io/homalg_project/HomalgToCAS/
Loading  Digraphs 0.13.0 (Digraphs - Methods for digraphs)
by Jan De Beule (http://homepages.vub.ac.be/~jdbeule/),
   Julius Jonusas (http://www-groups.mcs.st-andrews.ac.uk/~julius/),
   James Mitchell (http://goo.gl/ZtViV6),
   Michael Torpey (http://www-groups.mcs.st-andrews.ac.uk/~mct25/), and
   Wilf Wilson (http://wilf.me).
with contributions by:
   Stuart Burrell (sb235@st-andrews.ac.uk),
   Luke Elliott (le27@st-andrews.ac.uk),
   Christopher Jefferson (http://caj.host.cs.st-andrews.ac.uk),
   Markus Pfeiffer (https://www.morphism.de/~markusp/),
   Christopher Russell (cr66@st-andrews.ac.uk), and
   Finn Smith (fls3@st-andrews.ac.uk).
maintained by:
   James Mitchell (http://goo.gl/ZtViV6).
Homepage: https://gap-packages.github.io/Digraphs
Report issues at https://github.com/gap-packages/Digraphs/issues
Loading  Guarana 0.96.1 (Applications of Lie methods for computations with infinite polycyclic groups)
by Björn Assmann (bjoern@mcs.st-and.ac.uk).
with contributions by:
   John McDermott (jjm@mcs.st-and.ac.uk).
maintained by:
   The GAP Team (support@gap-system.org).
Homepage: https://gap-packages.github.io/guarana/
Report issues at https://github.com/gap-packages/guarana/issues

@fingolfin fingolfin added topic: packages issues or PRs related to package handling, or specific to a package (for packages w/o issue tracker) topic: library release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Jan 22, 2019
Copy link
Member

@olexandr-konovalov olexandr-konovalov left a comment

Choose a reason for hiding this comment

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

This is useful.

Copy link
Contributor

@ssiccha ssiccha left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Member

@frankluebeck frankluebeck left a 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.

Copy link
Contributor

@ThomasBreuer ThomasBreuer left a 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.

@ssiccha
Copy link
Contributor

ssiccha commented Jan 22, 2019

@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 UserPreference that tells GAP to print compact banners instead. Introducing that UserPreference would then be another issue.

@stevelinton
Copy link
Contributor

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.
@codecov
Copy link

codecov bot commented Jan 22, 2019

Codecov Report

Merging #3215 into master will decrease coverage by 8.97%.
The diff coverage is 100%.

@@            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
Impacted Files Coverage Δ
lib/package.gi 52.18% <100%> (-30.99%) ⬇️
src/macfloat.h 0% <0%> (-100%) ⬇️
src/fibhash.h 0% <0%> (-100%) ⬇️
src/syntaxtree.c 4.06% <0%> (-95.54%) ⬇️
lib/meatauto.gi 6% <0%> (-89.7%) ⬇️
lib/nilpquot.gi 11.11% <0%> (-88.89%) ⬇️
lib/helpt2t.gi 0.23% <0%> (-88.25%) ⬇️
src/modules.h 16.66% <0%> (-83.34%) ⬇️
src/dteval.c 4.19% <0%> (-79.23%) ⬇️
src/objset.c 8.49% <0%> (-76.64%) ⬇️
... and 439 more

@fingolfin
Copy link
Member Author

@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!

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0007%) to 84.643% when pulling 8bb3398 on fingolfin:mh/pkg-banner into 9d485d6 on gap-system:master.

@ThomasBreuer
Copy link
Contributor

@ssiccha I disagree.
When a user --in particular a user who is not familiar with GAP-- just wants to load a package
and gets several screens full of banners from this package and its needed and suggested packages
then I regard this neither as useful nor as information.
Altogether one line of version info for all the packages that got loaded would be more reasonable.

I like the idea of @stevelinton to show the long version of a package banner on demand.
In a classical GAP session, a function that shows the full banner could do that job.

@ssiccha
Copy link
Contributor

ssiccha commented Jan 23, 2019

@ThomasBreuer Ah, one could do something like print the compact banner per default and have a line like For information where to send bug reports and questions see PackageBannerLong("pkgname");. That's what I meant with "useful information": whom to contact.

Would that step on any package authors, maintainers, or contributors' toes since then their names won't be printed when loading a package?

@olexandr-konovalov
Copy link
Member

@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 Homepage and Report issues are very important, and do not throw them away.

@ChrisJefferson
Copy link
Contributor

ChrisJefferson commented Jan 23, 2019

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.

@olexandr-konovalov
Copy link
Member

olexandr-konovalov commented Jan 23, 2019

@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 Homepage and Report issues are very important, and do not throw them away.

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!

@ChrisJefferson
Copy link
Contributor

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?

@sebasguts
Copy link
Member

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 <details> tag.

@ThomasBreuer
Copy link
Contributor

I think that we have two issues.
First, we need something like a full package banner,
will all information that may be of interest, preferably with clickable links.
Second, we have to decide what should be shown by default as a side-effect
when a package gets loaded;
the ongoing discussion indicates that this should not be the full package banner.

Could we merge the current pull request,
which deals with the improvement of the full package banner,
and open a new issue for discussing the question about the default
side-effect when a package gets loaded?

@ThomasBreuer ThomasBreuer merged commit 3f0e2b2 into gap-system:master Jan 30, 2019
@fingolfin fingolfin deleted the mh/pkg-banner branch January 31, 2019 08:57
@DominikBernhardt DominikBernhardt added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Aug 22, 2019
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.11.0 milestone Feb 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: added PRs introducing changes that have since been mentioned in the release notes topic: library topic: packages issues or PRs related to package handling, or specific to a package (for packages w/o issue tracker)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants