Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#27946 closed enhancement (fixed)

libgap: distinct __str__ and __repr__ for GapElements

Reported by: embray Owned by:
Priority: major Milestone: sage-8.9
Component: interfaces Keywords: gap libgap
Cc: nthiery Merged in:
Authors: Erik Bray Reviewers: Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: 63db722 (Commits, GitHub, GitLab) Commit: 63db722182617741c5368166284ce0864b9776b2
Dependencies: Stopgaps:

Status badges

Description

Currently the libgap.GapElement objects have a __repr__ (but no explicit __str__, so the two are the same) which is taken from calling the GAP function ViewObj on the object and capturing and returning the result. This is essentially the same as when you display the object in a GAP interpreter prompt without using Print(), such as:

gap> a := "hello";
"hello"
gap> a;
"hello"

Although GAP has many options for displaying/printing variables, this makes ViewObj the closest possible thing to the __repr__ method on Python objects, which is called in similar circumstances:

>>> a = "hello"
>>> a
"hello"

This is in stark contrast with the pexpect interface's GapElement which (through a sequence of calls) effectively implements its __repr__ by calling Print(obj). Arguably this was not the best choice, but changing it might be tricky at this point. However, it does not explicitly implement __str__, so GapElement.__str__ does give a sensible result in this case.

Arguably Print(obj) is the right choice for __str__ since again in rough comparison with Python, when you print(...) an object in Python its __str__ is called.

This proposes to change just the libgap interface to meet this specification: libgap.GapElement.__str__ captures the output of Print(obj) and returns it. While libgap.GapElement.__repr__ captures and returns the output of ViewObj(obj).

This gives the best of both worlds, and better consistency in most contexts where GapElements? are converted to strings, making replacing pexpect gap with libgap easier and less disruptive.

Change History (5)

comment:1 Changed 2 years ago by embray

  • Authors set to Erik Bray
  • Branch set to u/embray/libs/gap/separate-str-repr
  • Cc nthiery added
  • Commit set to 63db722182617741c5368166284ce0864b9776b2
  • Status changed from new to needs_review

Here is a branch for this specific change that I extracted from the rest of my work on #18267, since this change is worth considering in its own right. Still needs a bit more testing to ensure it didn't inadvertently break any other tests.


New commits:

63db722Add a GapElement.__str__ that works the same as the pexpect GapElement._repr_ (and hence its __str__ as well).
Last edited 2 years ago by embray (previous) (diff)

comment:2 Changed 2 years ago by embray

Works without test failures for me.

comment:3 Changed 2 years ago by vdelecroix

  • Reviewers set to Vincent Delecroix
  • Status changed from needs_review to positive_review

comment:4 Changed 2 years ago by vbraun

  • Branch changed from u/embray/libs/gap/separate-str-repr to 63db722182617741c5368166284ce0864b9776b2
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:5 Changed 2 years ago by embray

  • Milestone changed from sage-8.8 to sage-8.9

Not in Sage 8.8. Let's please to try keep tickets' milestones related to the release in which we actually intend to include them, and in particular the release in which they were actually included, especially when closing tickets.

Note: See TracTickets for help on using tickets.