-
Notifications
You must be signed in to change notification settings - Fork 175
in general, do not use a known Name value in PrintObj
#5701
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
in general, do not use a known Name value in PrintObj
#5701
Conversation
|
No, apparently the CI tests do not complain about the proposed change. |
|
Would this really be a good idea? -- I mean, for example assume you name an object "X". Then, as I understand, with your proposal this would still be "Print"ed as "X", as there is a global variable named "X". (Or did I misread something?) |
Well, this has been the behaviour since more than 25 years, and nobody has reported a problem with it. I think that |
james-d-mitchell
left a comment
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 to me
frankluebeck
left a comment
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.
I do not support this new method. The way, an object is printed should not depend on the state of the GAP session in this way. (A global name can become bound and this may have no relation with the object to be printed.)
I would vote for removing this method completely.
Or, just leave it as it always was and adjust the documentation (if the additional work caused by removing it seems too much).
Also, I don't think that the proposed change addresses the issue #5699 appropriately. That should be done with better methods for viewing and printing those objects.
lib/object.gi
Outdated
| if ISBOUND_GLOBAL( Name( obj ) ) then | ||
| Print( Name( obj ) ); |
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.
I think this should only be triggered if the variable with that name actually contains the object. I.e. like so:
| if ISBOUND_GLOBAL( Name( obj ) ) then | |
| Print( Name( obj ) ); | |
| local name; | |
| name := Name( obj ); | |
| if ISBOUND_GLOBAL( name ) and IS_IDENTICAL_OBJ( VALUE_GLOBAL( name ), obj ) then | |
| Print( name ); |
lib/object.gi
Outdated
| [ HasName ], | ||
| SUM_FLAGS, # override anything specific | ||
| function ( obj ) Print( Name( obj ) ); end ); | ||
| function( obj ) |
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.
@ThomasBreuer perhaps less controversial: just remove this method, and add back dedicated printing methods for Rationals and other objects relying on this. This assumes there are only a few (Rationals, Integers, GaussianRationals, ... come to mind).
If there are sp many as to make this annoying, we could also introduce a new attribute NameForPrinting and use that in here.
That said, I wonder how badly this PR breaks package CI tests. Perhaps you can update and rebase this PR (resp. merge current master into it), then I could trigger a run of the PackageDistro CI tests against this pull request to see where it stands.
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.
O.k., I have updated the pull request (and also its description) accordingly.
Use a known `Name` value in `PrintObj` only if a global variable with this name exists.
- remove the offending `PrintObj` method - install individual `PrintObj` methods for the few objects that are affected (in our test suite) by this change
28a27ab to
54da6b8
Compare
|
Causes failures in the test suites of |
|
I see three different reasons for the failures in package tests:
I have no easy solution for the cases 2. and 3. At least some of the |
|
For the moment, I give up: |
This addresses #5699.
For more than 25 years, there is a
PrintObjmethod with high rank that just prints a knownNamevalue of the object in question.According to the documentation of
Name, a knownNamevalue should be used only byViewandViewObj; the functionPrintis recommended to show GAP input to reconstruct the object in question if this makes sense.When we remove the
PrintObjmethod in question, the tests fromtestinstallandteststandardshow differences essentially in two situations:Rationalsare currently expected to be printed via this name. The solution proposed here is to provideaindividualPrintObjmethod with high rank that uses a known name under the condition that a global variable with this name existsPrintObjmethods for the objects in question. This works because there are already filters that describe these objects.SimpleGroupsIteratorcallPrintin order to show the names of these groups. Here we can callViewinstead ofPrint.(Let us see the results of the other tests, perhaps there are more differences.)