Refactor some Map printing code #2877
Conversation
Reduce some code duplication and prepare for a further reduction in the future
fingolfin
requested review from
lgoettgens,
simonbrandhorst and
HechtiDerLachs
| @@ -71,7 +71,7 @@ end | |||
|
|
|||
| function Base.show(io::IO, ::MIME"text/plain", h::LieAlgebraHom) | |||
| io = pretty(io) | |||
| println(io, LowercaseOff(), "Lie algebra morphism") | |||
| println(IOContext(io, :supercompact => true), h) | |||
There was a problem hiding this comment.
This ensures that this detailed printing method is identical to that for LieAlgebraModuleHom or GAPGroupHomomorphism
| @@ -122,7 +122,9 @@ function Base.show(io::IOContext, Phi::MorphismFromRationalFunctions) | |||
| if get(io, :supercompact, false) | |||
| print("Morphism from rational functions") | |||
| else | |||
| print("Hom: ", X, " -> ", Y) | |||
There was a problem hiding this comment.
Fixes a bug, X and Y are not defined in this function
There was a problem hiding this comment.
(This also means there is no test for this printing variant, so ideally we should add one)
| @@ -122,7 +122,9 @@ function Base.show(io::IOContext, Phi::MorphismFromRationalFunctions) | |||
| if get(io, :supercompact, false) | |||
| print("Morphism from rational functions") | |||
| else | |||
| print("Hom: ", X, " -> ", Y) | |||
There was a problem hiding this comment.
(This also means there is no test for this printing variant, so ideally we should add one)
| print("Hom: ", X, " -> ", Y) | ||
| io = pretty(io) | ||
| print(io, "Hom: ") | ||
| print(io, Lowercase(), domain(Phi), " -> ", Lowercase(), codomain(Phi)) |
There was a problem hiding this comment.
In other functions we print this in "supercompact" mode. But not here and not in some others. One can argue either way, for now I wanted to keep this change "simple" while also aligning it as much as possible with other show methods for maps here and in other repos.
There was a problem hiding this comment.
I do not understand, which part is printed in "supercompact" in other functions ?
There was a problem hiding this comment.
This is "one line" printing mode, and the recommendation (though not a requirement) is that any delegation it performs uses "supercompact" mode, to help ensure the "one line" promise is kept. So for other kinds of maps we do
print(IOContext(io, :supercompact => true), Lowercase(), domain(x), " -> ", Lowercase(), codomain(x))
| @@ -19,7 +17,7 @@ function Base.show(io::IO, X::AbsProjectiveVariety{<:Field,<:MPolyQuoRing}) | |||
| elseif get_attribute(X, :is_empty, false) | |||
| print(io, "Empty projective variety") | |||
| else | |||
| print(io, "V(") | |||
| print(io, LowercaseOff(), "V(") | |||
There was a problem hiding this comment.
This ensures the V always is in uppercase, so technically this is a bug fix, albeit a very minor one
There was a problem hiding this comment.
Good to have this function, I was not aware of this one, thanks !
| psi = restricted_map(phi) | ||
| to = is_unicode_allowed() ? " ↦ " : " -> " | ||
| for i in 1:ngens(R)-1 | ||
| println(io, R[i], to, psi(R[i])) |
There was a problem hiding this comment.
This change is just about minimizing code duplication and simplification... though it also fixes a bug (in the unicode variant, there was a final println instead of a final print)
| print(io, "Hom: ") | ||
| if typeof(X) <: Union{PrincipalOpenSubset, AffineVariety{ <:Field, <: MPolyAnyRing}, <:Spec{<:Field, <:MPolyAnyRing}} # Take care of the case where the domain is not given as a V(bla) | ||
| print(io, Lowercase()) |
There was a problem hiding this comment.
This case was checking for specific types of X to decide whether or not to issue Lowercase() -- that's a bad code smell, don't do that! Here, the proper fix is to ensure that the sub-object issues LowercaseOff if necessary (I've done on such change elsewhere in this PR).
@StevellM There is more printing code for Schemes which performs similar checks that should be changed. I'll write to you about this separately on Slack.
There was a problem hiding this comment.
Good, as mentioned in another comment, I was not aware this was an option. I will add it to the list of things to be changed among others for the schemes' printings.
Codecov Report
@@ Coverage Diff @@
## master #2877 +/- ##
=======================================
Coverage 80.50% 80.51%
=======================================
Files 456 456
Lines 65214 65194 -20
=======================================
- Hits 52502 52491 -11
+ Misses 12712 12703 -9
|
Reduce some code duplication and prepare for a further reduction in the future.
The primary goal here is to remove gratuitous differences in the code, which in turn makes it trivial to detect possibly unwanted differences and other problematic behavior. I'll add some inline comments to point out specific changes.
See also Nemocas/AbstractAlgebra.jl#1450 for related changes in AbstractAlgebra.
CC @StevellM