-
Notifications
You must be signed in to change notification settings - Fork 161
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
Extend StreamFormatting to seperate linewrap and indenting #5140
base: master
Are you sure you want to change the base?
Conversation
36e8a65
to
62ff897
Compare
I think you need to run |
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.
Seems generally sensible to me. Of course as you say, it stills needs to be documented, tests adjusted etc. But it makes sense to first gather feedback, e.g. from @frankluebeck and @ThomasBreuer
15d285f
to
42289b2
Compare
2223592
to
30515f9
Compare
30515f9
to
6f4d903
Compare
I've rebased this PR, after a bunch of changes / reformattings. It still lacks documentation, which I'm happy to write, but would like to know if anyone is fundamentally opposed to the idea first. |
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 (still) like this. Of course docs are needed. Some minor comments.
One more ping to @frankluebeck and @ThomasBreuer in case they would like to comment
d3daae3
to
511c041
Compare
08b1dd0
to
3db2723
Compare
I've cleaned up this PR so it applies to master, and added some basic documentation. I removed (most) of the whitespace-only changes, which should make the PR easier to read, hopefully. This is quite a significant change to a fairly fundamental part of GAP (how indenting and line wrapping works), so there might be some fallout I haven't noticed. |
I'm not sure if the following is the expected behaviour:
I would expect the return value to be a record. |
7427745
to
341e433
Compare
Started a package distro CI run against this PR, logs at https://github.com/gap-system/PackageDistro/actions/runs/7589478131 |
Semigroups package tests fail but I'm happy to make a release and update things to accommodate this Pr |
Some of that output looks really strange, with very strange wrapping -- should probably try to figure out what's going on, as the only change I would expect from this would be if you actually tried to print out the value of |
e532387
to
1580857
Compare
As indenting and line wrapping hints are implemented teogether, we always track indent and linewrapping hints, but only make use of them when indenting, or linewrapping, are enabled.
1580857
to
e6956ad
Compare
This is another attempt at an idea previous discussed in #4496 and #4511
This allows passing a record to SetPrintFormattingStatus with separate
linewrap
andindent
options.PrintFormattingStatus
now always returns a record.This code needs obvious documentation (help welcome!) but I thought I'd see if it merges, and the direction seems basically sensible.
After this we may want to make further changes, for example disabling
linewrap
by default for things likeOutputTextFile
, but that can be a separate PR.