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

Extend StreamFormatting to seperate linewrap and indenting #5140

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ChrisJefferson
Copy link
Contributor

This is another attempt at an idea previous discussed in #4496 and #4511

This allows passing a record to SetPrintFormattingStatus with separate linewrap and indent 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 like OutputTextFile, but that can be a separate PR.

@ChrisJefferson ChrisJefferson added the do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) label Oct 19, 2022
@ChrisJefferson ChrisJefferson force-pushed the formatting branch 2 times, most recently from 36e8a65 to 62ff897 Compare October 19, 2022 16:53
@fingolfin
Copy link
Member

I think you need to run TestDirectory("tst/testinstall", rec(rewriteToFile:=true));

Copy link
Member

@fingolfin fingolfin left a 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

lib/streams.gi Outdated Show resolved Hide resolved
src/io.c Outdated Show resolved Hide resolved
src/io.h Outdated Show resolved Hide resolved
src/io.c Outdated Show resolved Hide resolved
src/io.c Outdated Show resolved Hide resolved
@ChrisJefferson ChrisJefferson force-pushed the formatting branch 5 times, most recently from 15d285f to 42289b2 Compare October 20, 2022 09:28
@fingolfin fingolfin added the gapdays2022-summer Issues and PRs that arose at https://www.gapdays.de/gapdays2022-summer label Oct 20, 2022
lib/streams.gi Outdated Show resolved Hide resolved
@ChrisJefferson ChrisJefferson removed the do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) label Nov 2, 2022
@ChrisJefferson
Copy link
Contributor Author

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.

Copy link
Member

@fingolfin fingolfin left a 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

lib/streams.gi Outdated Show resolved Hide resolved
lib/streams.gi Outdated Show resolved Hide resolved
lib/streams.gi Outdated Show resolved Hide resolved
src/io.c Outdated Show resolved Hide resolved
src/io.h Outdated Show resolved Hide resolved
@ChrisJefferson
Copy link
Contributor Author

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.

@zickgraf
Copy link
Contributor

I'm not sure if the following is the expected behaviour:

gap> PrintFormattingStatus(OutputTextString("",false));
true

I would expect the return value to be a record.

@ChrisJefferson ChrisJefferson force-pushed the formatting branch 4 times, most recently from 7427745 to 341e433 Compare January 18, 2024 04:00
@fingolfin
Copy link
Member

Started a package distro CI run against this PR, logs at https://github.com/gap-system/PackageDistro/actions/runs/7589478131

@james-d-mitchell
Copy link
Contributor

Semigroups package tests fail but I'm happy to make a release and update things to accommodate this Pr

@ChrisJefferson
Copy link
Contributor Author

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 PrintFormattingStatus, and saw it's now a record instead of a boolean.

@ChrisJefferson ChrisJefferson force-pushed the formatting branch 4 times, most recently from e532387 to 1580857 Compare January 23, 2024 12:25
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gapdays2022-summer Issues and PRs that arose at https://www.gapdays.de/gapdays2022-summer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants