Introduce VarName

[mgkurtz]

As discussed in #1282 (comment), this introduces

const VarName = Union{Symbol, AbstractString, Char}

Hopefully I found all places, where this should be used. In some places I use VarNameNonSymbol instead to prohibit infinite recursion.

In a separate PR, we can care for different ways to provide lists of variable names. For now, they are changed to AbstractVector{<:VarName}.

[mgkurtz]

It seems, like where I now wrote foo(s::VarName) = Generic.foo(Symbol(s)), I need to provide foo(s::VarNameNonSymbol) = foo(Symbol(s)) as to allow other implementations to implement only foo(::Symbol). So, I actually removed to much. On the other hand, I am once more inclined to use some metaprogramming and write

@add_var_name_method foo Generic.foo (a, s::VarName=var(a), b)

to provide

foo(a, s::Symbol, b) = Generic.foo(a, s, b)
foo(a, s::VarName=var(a), b) = foo(a, Symbol(s), b)

With a similar interface for lists of variable names. That way, I could remove some more noise from the files. In the case of variable name lists, this approach could further help, if we later decide on adding more flexibility there.

[codecov]

Codecov Report

Merging #1291 (4cd578a) into master (8aefeec) will increase coverage by 0.26%.
The diff coverage is 84.61%.

@@            Coverage Diff             @@
##           master    #1291      +/-   ##
==========================================
+ Coverage   85.51%   85.77%   +0.26%     
==========================================
  Files         102      102              
  Lines       27987    27855     -132     
==========================================
- Hits        23932    23893      -39     
+ Misses       4055     3962      -93     

Impacted Files	Coverage Δ
src/FreeAssAlgebra.jl	98.93% <ø> (ø)
src/Rings.jl	51.78% <0.00%> (-2.94%)	⬇️
src/fundamental_interface.jl	100.00% <ø> (ø)
src/generic/MPoly.jl	95.08% <ø> (+0.16%)	⬆️
src/generic/NCPoly.jl	61.00% <ø> (-0.96%)	⬇️
src/generic/Poly.jl	98.75% <ø> (-0.03%)	⬇️
src/AbsSeries.jl	93.36% <60.00%> (+2.54%)	⬆️
src/RelSeries.jl	93.52% <60.00%> (+1.72%)	⬆️
src/NCRings.jl	89.13% <66.66%> (-1.78%)	⬇️
src/MPoly.jl	78.13% <90.00%> (+0.65%)	⬆️
... and 13 more

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[mgkurtz]

The missing test coverage is unrelated to this issue. We could improve the coverage by factoring out parts of the code instead of having it copied all around. But, as said, this is unrelated.

[fingolfin]

I like it. OscarCI is failing, though? Seems to be related to some Singular stuff?

Meta question: do we really want/need that Char in there? Is anyone really using 'x' instead of "x"? And even if they are, do we want to encourage that? It seems to just complicate things further.

Regarding AbstractString: if we want to support general strings, there should be test cases testing those. There is Test.GenericString for that purpose.

[thofma]

Yes, people are using Char. I remember various complaints. It should be kept.

[fingolfin]

Stupid question, but: what is t here? Is this another "symbol"?

The documentation says:

The string fields are currently ignored, but are reserved for future use.

But what's the point of that?! The arguments are completely unspecified, after all. Maybe they are matching something in Nemo or Hecke? But I don't see an argument t in the OSCAR docstrings for these functions either?

(This is of course not a blocker for this PR, I just noticed it while reviewing the PR)

[thofma]

This function will never be called in OSCAR/Nemo/Hecke :)

[mgkurtz]

The test failure seems unrelated. Here is a new issue for that #1293.