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

Bugfix: Convert numbers to Int in extendedterminfo #51195

Merged
merged 1 commit into from
Sep 5, 2023

Conversation

jakobnissen
Copy link
Contributor

Before, in the extendedterminfo function in terminfo.jl, if the numbers array was nonempty, the function would fail as a UInt32 cannot be implicitly converted to the output Int type. Do this conversion explicitly.

Closes #51190

@tecosaur it seems that this function was not tested. Looking at code coverage, base/terminfo.jl only has 18% coverage. Would it be possible to test this functionality more thoroughly?

Before, in the extendedterminfo function in terminfo.jl, if the numbers array
was nonempty, the function would fail as a `UInt32` cannot be implicitly
converted to the output `Int` type. Do this conversion explicitly.
@tecosaur
Copy link
Contributor

tecosaur commented Sep 5, 2023

Hi Jacob, I've been thinking about #51190 and I have a number of improvements to terminfo.jl worked out, but I haven't been able to figure out what's going on with screen.xterm-256color.

Is this change equivalent to changing NumInt to Int16/Int32? Because I have that as part of my unpushed changes and I still have issues with screen.xterm-256color.

If it helps, here's my WIP-revised terminfo.jl: http://ix.io/4Fre

@tecosaur
Copy link
Contributor

tecosaur commented Sep 5, 2023

Regarding the supposed 18% code coverage, I'm a bit suspicious of that. It says read(data::IO, ::Type{TermInfoRaw}) is uncalled but we have Base.TermInfo(read(IOBuffer(xterm_terminfo), Base.TermInfoRaw)) in test/terminfo.jl.

@tecosaur
Copy link
Contributor

tecosaur commented Sep 5, 2023

Oh, I see that #51190 isn't the bug I was thinking of (#51110).

@jakobnissen
Copy link
Contributor Author

No this bug is much more trivial. The output of extendedterminfo is a Dict{Symbol, Union{Bool, Int, String}}, where the values also include the values of number::Vector{NumInt}. But this fails because you can't convert NumType to the value type of the dict:

julia> Dict{String, Union{Int, String}}("1"=>UInt32(1))
ERROR: MethodError: Cannot `convert` an object of type
  UInt32 to an object of type
  Union{Int64, String}

The real problem is that the function was not tested.

@tecosaur
Copy link
Contributor

tecosaur commented Sep 5, 2023

In test/terminfo.jl the xterm extended capabilities are tested. They might all be strings though...

@jakobnissen
Copy link
Contributor Author

Ah I see. Not sure what's up with the coverage then

@tecosaur
Copy link
Contributor

tecosaur commented Sep 5, 2023

I did think I added it to choosetests.jl correctly...

@tecosaur
Copy link
Contributor

tecosaur commented Sep 5, 2023

I've now put up my WIP changes as #51198.

@jakobnissen jakobnissen added the merge me PR is reviewed. Merge when all tests are passing label Sep 5, 2023
@jakobnissen
Copy link
Contributor Author

Added the merge me label because this is a straightforward bugfix

@vtjnash vtjnash merged commit c84324f into JuliaLang:master Sep 5, 2023
@oscardssmith oscardssmith added bugfix This change fixes an existing bug and removed merge me PR is reviewed. Merge when all tests are passing labels Sep 5, 2023
@jakobnissen jakobnissen deleted the termint branch September 5, 2023 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

terminfo parser: extendedterminfo() results in conversion error for TERM=screen-256color
4 participants