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

Add terse and is_terse helpers, and use them #1677

Merged
merged 8 commits into from
Apr 19, 2024

Conversation

fingolfin
Copy link
Member

I deliberately did not yet try to convert any uses of :compact, to make this non-breaking.

This will conflict with PR #1671 but so be it, I can rebase that one.

lgoettgens

This comment was marked as resolved.

@lgoettgens
Copy link
Collaborator

Complicating matters, there already is a :terse flag in AA, see

terse_level = get(io, :terse, false) ? 1 : 0

and for the effect
@test sprint(show, p) == "a + b^2 + c^3"
@test sprint(show, p, context = :compact => true) == "a + b^2 + c^3"
@test sprint(show, p, context = :terse => true) == "a+b^2+c^3"

How do we handle this in the context of this new terse naming?

Copy link

codecov bot commented Apr 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.93%. Comparing base (795382d) to head (4eb3fbf).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1677   +/-   ##
=======================================
  Coverage   86.92%   86.93%           
=======================================
  Files         116      116           
  Lines       29622    29624    +2     
=======================================
+ Hits        25750    25753    +3     
+ Misses       3872     3871    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fingolfin
Copy link
Member Author

fingolfin commented Apr 18, 2024

Oh wow, I was not aware of the existing :terse, and the isterse function, at all. Thanks for noticing it.

On the positive side, nothing outside of AA in our stack seems to reference them (there are some occurrences of :terse in Nemo but this s unrelated).

Note that :terse is just used to set the initial value for terse_level (and isterse tests whether the terse level is positive).
But nothing besides a single test seems to set :terse? So is this an unused feature? Or perhaps I missed something using it?

There also is something fishy about the code modifying the `terse_level`: ```julia function set_terse(S::printer) S.terse_level = max(1, S.terse_level + 1) end

function restore_terse(S::printer)
S.terse_level = S.terse_level - 1
end

Note the assymetry: the function increasing the `terse_level` imposes an upper bound of 1, while the function decreasing it does not impose a lower bound... This could conceivable lead to an underrun ? Assuming the two functions are always called in pairs...?
</s>

UPDATE: Nevermind, I just totally misunderstood the code in `set_terse` *sigh*. It sets a *lower* bound of 1, not an upper bound. Ugh.


Anyway, if we drop the use of `:terse` in the IOContext (which seems to be not used by anything), all should be fine?

@fingolfin
Copy link
Member Author

I think PRs #1679 and #1678 take care of the "clash" with names of existing purely internal code.

@fingolfin fingolfin merged commit 58016e1 into Nemocas:master Apr 19, 2024
29 of 31 checks passed
@fingolfin fingolfin deleted the mh/terse branch April 19, 2024 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants