Better show #125

aminya · 2019-10-28T10:52:31Z

Edited show method to not show the node.ptr (which includes uninformative/unintuitive pointer memory location number).

This also solves the error of not having a method for zero(::Ptr{EzXML._Node})
https://github.com/JuliaMusic/MusicXML.jl/runs/435363039?check_suite_focus=true#step:5:58

There is no single package I have ever used that shows the address of the memory. Julia is not a language for showing what happens at low-level (pointers, memory locations, etc).

All the other types in Julia use the following information for showing a variable:

Type of the variable
The content, elements, children, etc
Size or length of the variable (if exists)

If you think that the `show` needs to be more informative, then we can print the content too.

Similar packages show the content of the Node:

WebIO:

julia> Node(:div, Node(:p, "I am a paragraph!", class="important"))
(div
  (p { class="important" }
    "I am a paragraph!"))

https://juliagizmos.github.io/WebIO.jl/latest/api/node/#WebIO.Node

Hyperscript:

julia> m("div", class="entry",

           m("h1", "An Important Announcement"))
<div class="entry"><h1>An Important Announcement</h1></div>

https://github.com/yurivish/Hyperscript.jl

LightXML:

julia> xdoc = parse_file("ex1.xml");
<?xml version="1.0" encoding="utf-8"?>
<bookstore>
  <book category="COOKING" tag="first">
    <title lang="en">Everyday Italian</title>
    <author>Giada De Laurentiis</author>
    <year>2005</year>
    <price>30.00</price>
  </book>
  <book category="CHILDREN">
    <title lang="en">Harry Potter</title>
    <author>J K. Rowling</author>
    <year>2005</year>
    <price>29.99</price>
  </book>
</bookstore>

https://github.com/JuliaIO/LightXML.jl

LibExpat

julia> pd = xp_parse(open(f -> read(f, String), "t_s1.xml"))
<xs:simpleType name="setDigitalPointType">
    <xs:union>
        <xs:simpleType>
            <xs:restriction base="xs:string">
                <xs:enumeration value="on"/>
                <xs:enumeration value="off"/>
            </xs:restriction>
        </xs:simpleType>
        <xs:simpleType>
            <xs:restriction base="xs:string">
                <xs:pattern value="[Oo][Nn]"/>
                <xs:pattern value="[Oo][Ff][Ff]"/>
            </xs:restriction>
        </xs:simpleType>
    </xs:union>
</xs:simpleType>

https://github.com/JuliaIO/LibExpat.jl

I would like to hear others' ideas on this too.

codecov-io · 2020-02-10T04:56:58Z

Codecov Report

Merging #125 into master will increase coverage by 0.34%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #125      +/-   ##
==========================================
+ Coverage   95.24%   95.58%   +0.34%     
==========================================
  Files           7        7              
  Lines         715      748      +33     
==========================================
+ Hits          681      715      +34     
+ Misses         34       33       -1

Impacted Files	Coverage Δ
src/EzXML.jl	`100% <ø> (ø)`	⬆️
src/node.jl	`95.87% <100%> (+0.34%)`	⬆️
src/streamreader.jl	`93.1% <100%> (+0.29%)`	⬆️
src/document.jl	`97.08% <100%> (+0.38%)`	⬆️
src/buffer.jl	`100% <0%> (ø)`	⬆️
src/xpath.jl	`95.23% <0%> (+0.11%)`	⬆️
src/error.jl	`96.55% <0%> (+0.12%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 76d62ce...9af7c7e. Read the comment docs.

aminya · 2020-02-28T06:44:18Z

@bicycle1885 Could you merge this? This show-error makes the package unusable.

 MethodError: no method matching zero(::Ptr{EzXML._Node})
  Closest candidates are:
    zero(!Matched::Type{Missing}) at missing.jl:103
    zero(!Matched::Type{LibGit2.GitHash}) at D:\buildbot\worker\package_win64\build\usr\share\julia\stdlib\v1.4\LibGit2\src\oid.jl:220    zero(!Matched::Type{Pkg.Resolve.VersionWeight}) at D:\buildbot\worker\package_win64\build\usr\share\julia\stdlib\v1.4\Pkg\src\Resolve\versionweights.jl:15
    ...
  Stacktrace:
   [1] unsigned(::Ptr{EzXML._Node}) at .\int.jl:158
   [2] show(::IOContext{Base.GenericIOBuffer{Array{UInt8,1}}}, ::Node) at C:\Users\yahyaaba\.julia\packages\EzXML\ZNwhK\src\node.jl:306
   [3] show_delim_array(::IOContext{Base.GenericIOBuffer{Array{UInt8,1}}}, ::Tuple{Node}, ::Char, ::Char, ::Char, ::Bool, ::Int64, ::Int64) at .\show.jl:748
   [4] show_delim_array at .\show.jl:733 [inlined]
   [5] show(::IOContext{Base.GenericIOBuffer{Array{UInt8,1}}}, ::Tuple{Node}) at .\show.jl:766
   [6] _show_default(::IOContext{Base.GenericIOBuffer{Array{UInt8,1}}}, ::Any) at .\show.jl:394
   [7] show_default at .\show.jl:377 [inlined]
   [8] show(::IOContext{Base.GenericIOBuffer{Array{UInt8,1}}}, ::Any) at .\show.jl:374
   [9] sprint(::Function, ::MethodError; context::Pair{Symbol,Bool}, sizehint::Int64) at .\strings\io.jl:103
   [10] Test.Error(::Any, ::Any, ::Any, ::Any, ::Any) at D:\buildbot\worker\package_win64\build\usr\share\julia\stdlib\v1.4\Test\src\Test.jl:162
   [11] do_test(::Test.ExecutionResult, ::Any) at D:\buildbot\worker\package_win64\build\usr\share\julia\stdlib\v1.4\Test\src\Test.jl:518
   [12] top-level scope at C:\Users\yahyaaba\Documents\GitHub\AcuteML.jl\test\xmlutils.jl:135
   [13] top-level scope at D:\buildbot\worker\package_win64\build\usr\share\julia\stdlib\v1.4\Test\src\Test.jl:1113
   [14] top-level scope at C:\Users\yahyaaba\Documents\GitHub\AcuteML.jl\test\xmlutils.jl:84
   [15] top-level scope at D:\buildbot\worker\package_win64\build\usr\share\julia\stdlib\v1.4\Test\src\Test.jl:1113
   [16] top-level scope at C:\Users\yahyaaba\Documents\GitHub\AcuteML.jl\test\xmlutils.jl:4
   [17] include(::String) at .\client.jl:439
   [18] top-level scope at C:\Users\yahyaaba\Documents\GitHub\AcuteML.jl\test\runtests.jl:41
   [19] include_string(::Module, ::String, ::String) at .\loading.jl:1080
   [20] (::Atom.var"#198#202"{String,String})() at C:\Users\yahyaaba\.julia\packages\Atom\N5oSJ\src\eval.jl:127
   [21] withpath(::Atom.var"#198#202"{String,String}, ::String) at 
C:\Users\yahyaaba\.julia\packages\CodeTools\kosGY\src\utils.jl:30  
   [22] withpath(::Function, ::String) at C:\Users\yahyaaba\.julia\packages\Atom\N5oSJ\src\eval.jl:9
   [23] #197 at C:\Users\yahyaaba\.julia\packages\Atom\N5oSJ\src\eval.jl:124 [inlined]
   [24] with_logstate(::Atom.var"#197#201"{String,String}, ::Base.CoreLogging.LogState) at .\logging.jl:398
   [25] with_logger at .\logging.jl:505 [inlined]
   [26] #196 at C:\Users\yahyaaba\.julia\packages\Atom\N5oSJ\src\eval.jl:123 [inlined]
   [27] hideprompt(::Atom.var"#196#200"{String,String}) at C:\Users\yahyaaba\.julia\packages\Atom\N5oSJ\src\repl.jl:122
   [28] macro expansion at C:\Users\yahyaaba\.julia\packages\Atom\N5oSJ\src\eval.jl:122 [inlined]
   [29] macro expansion at C:\Users\yahyaaba\.julia\packages\Media\ItEPc\src\dynamic.jl:24 [inlined]
   [30] evalall(::String, ::Nothing, ::String) at C:\Users\yahyaaba\.julia\packages\Atom\N5oSJ\src\eval.jl:112
   [31] (::Atom.var"#194#195")(::Dict{String,Any}) at C:\Users\yahyaaba\.julia\packages\Atom\N5oSJ\src\eval.jl:106
   [32] handlemsg(::Dict{String,Any}, ::Dict{String,Any}) at C:\Users\yahyaaba\.julia\packages\Atom\N5oSJ\src\comm.jl:166
   [33] (::Atom.var"#19#21"{Array{Any,1}})() at .\task.jl:358

DilumAluthge · 2020-02-29T08:41:26Z

Bump @bicycle1885

Fix tests

bicycle1885 · 2020-03-01T00:02:11Z

Edited show method to not show the node.ptr (which includes uninformative/unintuitive pointer memory location number).

I do not think pointers are uninformative because they are identifiers in a document tree. For example, if you get two nodes in a tree in two different ways and their pointers are identical, these pointers quickly tell us that those two nodes are identical.

Method error of zero(::Ptr{EzXML._Node}) seems to be orthogonal to the way of showing nodes. I believe you will never see nodes with null pointers. Can you elaborate on the situation?

aminya · 2020-03-01T00:16:50Z

I do not think pointers are uninformative because they are identifiers in a document tree. For example, if you get two nodes in a tree in two different ways and their pointers are identical, these pointers quickly tell us that those two nodes are identical.

Nowhere else in Julia, anyone shows the address of memory for any type of variable.
One can easily do node1 == node2 to check if two nodes are similar instead of using the address of memory for checking if two variables are the same.

Method error of zero(::Ptr{EzXML._Node}) seems to be orthogonal to the way of showing nodes. I believe you will never see nodes with null pointers. Can you elaborate on the situation?

You can see the error by running the tests on Julia 1.4. It is caused by showing node.ptr in show methods.

bicycle1885 · 2020-03-01T02:36:06Z

If you have several node objects in your hand, you can quickly check their distinctiveness by looking at their pointer identifiers. If no pointers shown, you need to manually check comparison using == or similar. I don't think losing printed information will make printed nodes more informative or intuitive. Because the space for printing is limited, printing pointers is a reasonable way to show useful information to users.

You can see the error by running the tests on Julia 1.4. It is caused by showing node.ptr in show methods.

I think this is a bug introduced in Julia 1.4, so I sent a pull request to the upstream (JuliaLang/julia#34941).

aminya · 2020-03-01T03:07:35Z

If you have several node objects in your hand, you can quickly check their distinctiveness by looking at their pointer identifiers. If no pointers shown, you need to manually check comparison using == or similar. I don't think losing printed information will make printed nodes more informative or intuitive. Because the space for printing is limited, printing pointers is a reasonable way to show useful information to users.

All the other types in Julia use the following information for showing a variable:

Type of the variable
The content, elements, children, etc
Size or length of the variable (if exists)

If you think that the `show` needs to be more informative, then we should print the content.

There is no single package I have ever used that shows the address of the memory. Julia is not a language for showing what happens at low-level (pointers, memory locations, etc).

Similar packages show the content of the Node:

WebIO:

julia> Node(:div, Node(:p, "I am a paragraph!", class="important"))
(div
  (p { class="important" }
    "I am a paragraph!"))

https://juliagizmos.github.io/WebIO.jl/latest/api/node/#WebIO.Node

Hyperscript:

julia> m("div", class="entry",

           m("h1", "An Important Announcement"))
<div class="entry"><h1>An Important Announcement</h1></div>

https://github.com/yurivish/Hyperscript.jl

LightXML:

julia> xdoc = parse_file("ex1.xml");
<?xml version="1.0" encoding="utf-8"?>
<bookstore>
  <book category="COOKING" tag="first">
    <title lang="en">Everyday Italian</title>
    <author>Giada De Laurentiis</author>
    <year>2005</year>
    <price>30.00</price>
  </book>
  <book category="CHILDREN">
    <title lang="en">Harry Potter</title>
    <author>J K. Rowling</author>
    <year>2005</year>
    <price>29.99</price>
  </book>
</bookstore>

https://github.com/JuliaIO/LightXML.jl

LibExpat

julia> pd = xp_parse(open(f -> read(f, String), "t_s1.xml"))
<xs:simpleType name="setDigitalPointType">
    <xs:union>
        <xs:simpleType>
            <xs:restriction base="xs:string">
                <xs:enumeration value="on"/>
                <xs:enumeration value="off"/>
            </xs:restriction>
        </xs:simpleType>
        <xs:simpleType>
            <xs:restriction base="xs:string">
                <xs:pattern value="[Oo][Nn]"/>
                <xs:pattern value="[Oo][Ff][Ff]"/>
            </xs:restriction>
        </xs:simpleType>
    </xs:union>
</xs:simpleType>

https://github.com/JuliaIO/LibExpat.jl

I would like to hear others' ideas on this too.

bicycle1885 · 2020-03-01T07:02:17Z

I do not think that the current way is the best, but I do not think that just removing pointers makes it better, neither. Any suggestions of printing nodes are welcomed.

aminya · 2020-03-01T07:03:49Z

I do not think that the current way is the best, but I do not think that just removing pointers makes it better, neither. Any suggestions of printing nodes are welcomed.

Should we follow Julia's high-level convention and print the content instead? After showing the type?

julia> v = [1, 2, 3]
3-element Array{Int64,1}:
 1
 2
 3

KristofferC · 2020-03-01T12:00:09Z

@aminya, as a tip, it is usually better for a PR to focus on one thing. This changes the printing and works around the Julia bug which are two things. A better way forward is probably to make a PR that work around the Julia bug without changing the printing so that a new version working with 1.4 can be released. Then a discussion about the exact printing style can be discussed separately.

aminya requested a review from bicycle1885 February 10, 2020 04:26

aminya force-pushed the betterShow branch 3 times, most recently from 3eab54b to 8bf0b7b Compare February 10, 2020 04:42

aminya added a commit to aminya/AcuteML.jl that referenced this pull request Feb 28, 2020

to prevent EzXML error: JuliaIO/EzXML.jl#125

776296d

aminya added 4 commits February 29, 2020 17:20

Remove node.ptr from show

ad6c3dd

Fix tests

Remove Ptr from StreamReader

8778e29

using print instead of @printf

8fa3af1

Faster show

bcdcf57

aminya force-pushed the betterShow branch from 7f17b6f to bcdcf57 Compare February 29, 2020 23:21

bicycle1885 mentioned this pull request Mar 1, 2020

fix unsigned(::Ptr) and signed(::Ptr) JuliaLang/julia#34941

Merged

No EzXML prefix in showing

eefee4e

aminya force-pushed the betterShow branch from f37b45f to 1cc8cc7 Compare March 1, 2020 02:51

aminya added 2 commits February 29, 2020 21:12

show for Document

746aee0

Update docs

9af7c7e

aminya force-pushed the betterShow branch from fb4ed8b to 9af7c7e Compare March 1, 2020 03:13

felipenoris mentioned this pull request Mar 1, 2020

New package: EzXMLPatched v1.0.0 JuliaRegistries/General#10325

Closed

aminya force-pushed the betterShow branch from 21a0db6 to 9af7c7e Compare March 1, 2020 05:30

aminya added a commit to aminya/AcuteML.jl that referenced this pull request Mar 1, 2020

function for EzXML error preventation JuliaIO/EzXML.jl#125

fbbef4d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better show #125

Better show #125

aminya commented Oct 28, 2019 •

edited

Loading

codecov-io commented Feb 10, 2020 •

edited

Loading

aminya commented Feb 28, 2020 •

edited

Loading

DilumAluthge commented Feb 29, 2020

bicycle1885 commented Mar 1, 2020

aminya commented Mar 1, 2020

bicycle1885 commented Mar 1, 2020

aminya commented Mar 1, 2020 •

edited

Loading

bicycle1885 commented Mar 1, 2020

aminya commented Mar 1, 2020 •

edited

Loading

KristofferC commented Mar 1, 2020

Better show #125

Are you sure you want to change the base?

Better show #125

Conversation

aminya commented Oct 28, 2019 • edited Loading

If you think that the show needs to be more informative, then we can print the content too.

codecov-io commented Feb 10, 2020 • edited Loading

Codecov Report

aminya commented Feb 28, 2020 • edited Loading

DilumAluthge commented Feb 29, 2020

bicycle1885 commented Mar 1, 2020

aminya commented Mar 1, 2020

bicycle1885 commented Mar 1, 2020

aminya commented Mar 1, 2020 • edited Loading

If you think that the show needs to be more informative, then we should print the content.

bicycle1885 commented Mar 1, 2020

aminya commented Mar 1, 2020 • edited Loading

KristofferC commented Mar 1, 2020

aminya commented Oct 28, 2019 •

edited

Loading

If you think that the `show` needs to be more informative, then we can print the content too.

codecov-io commented Feb 10, 2020 •

edited

Loading

aminya commented Feb 28, 2020 •

edited

Loading

aminya commented Mar 1, 2020 •

edited

Loading

If you think that the `show` needs to be more informative, then we should print the content.

aminya commented Mar 1, 2020 •

edited

Loading