Add Attribute Functions for the SteamReader #90

programmeroftheeve · 2018-08-28T22:00:53Z

Adds functions for accessing attributes with the StreamReader without having to expand the tree.

Features:

Getting Values from the XML Events (Attributes don't have content, they have values)
- hasnodevalue
- nodevalue
Getting Attributes with "name" or numeric index
Getting a Julia Dictionary of the attribute names and values
Added AttributeReader Iterator
Get Number of Attributes
Check if Attributes Exist

Todo:

Add Documentation

codecov-io · 2018-08-28T22:15:09Z

Codecov Report

Merging #90 into master will increase coverage by 0.11%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #90      +/-   ##
==========================================
+ Coverage   98.89%   99.01%   +0.11%     
==========================================
  Files           7        7              
  Lines         545      608      +63     
==========================================
+ Hits          539      602      +63     
  Misses          6        6

Impacted Files	Coverage Δ
src/EzXML.jl	`100% <ø> (ø)`	⬆️
src/streamreader.jl	`98.66% <100%> (+1.1%)`	⬆️

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 3751fbe...360dfb6. Read the comment docs.

bicycle1885

This looks great! I really appreciate your work, but it still need some minor modifications as I commented.

bicycle1885 · 2018-08-30T10:20:14Z

src/streamreader.jl

@@ -43,6 +43,7 @@ else
    @assert false "invalid Cint size"
 end

+


This blank line is no needed.

bicycle1885 · 2018-08-30T10:21:50Z

src/streamreader.jl

@@ -55,6 +56,8 @@ function Base.convert(::Type{T}, x::ReaderType) where {T<:Integer}
    return convert(T, reinterpret(Cint, x))
 end

+Base.hash(x::ReaderType) = hash(convert(Cint,x))


You should implement the two-argument hash method (i.e. hash(x::ReaderType, h::UInt)); see the docstring of hash.

bicycle1885 · 2018-08-30T10:49:51Z

src/streamreader.jl

@@ -279,6 +324,8 @@ end
    nodecontent(reader::StreamReader)

 Return the content of the current node of `reader`.
+
+Note: Nodes of `READER_ATTRIBUTE` will crash with `nodecontent`, use `nodevalue` instead.


How can we reach an attribute node while reading an XML doc? Or, do we really need to notify users of this case?

An attribute node can be reached with the eachattribute function called on a StreamReader. If need, there can be an ArgumentError thrown when this is called on a attribute node

But eachattribute returns an AttributeReader object, no?

Anyway, I think throwing an ArgumentError exception would be better than crashing the process.

That is true

bicycle1885 · 2018-08-30T10:51:16Z

src/streamreader.jl

+       Cint,
+       (Ptr{Cvoid},),
+       reader.ptr)
+    return r == 1


Asserting that r is non-negative (@assert r >= 0) would be nice.

bicycle1885 · 2018-08-30T10:54:33Z

src/streamreader.jl

+        (Ptr{Cvoid},),
+        reader.ptr)
+    if value_ptr == C_NULL
+        return nothing


In this case, it should throw an ArgumentError exception like other accessor functions. We may change this behavior in the future, but I think we should adhere to consistency for now.

bicycle1885 · 2018-08-30T11:04:07Z

src/streamreader.jl

+"""
+    nodeattributecount(reader::StreamReader)
+
+Return a AttributeReader for the current node of `reader`


a AttributeReader => an `AttributeReader`

bicycle1885 · 2018-08-30T11:06:07Z

src/streamreader.jl

+
+Return the number of attributes in the current node of `reader`.
+"""
+function nodeattributecount(reader::StreamReader)


Overloading the countattributes function defined in src/node.jl would be better.

bicycle1885 · 2018-08-30T11:10:40Z

test/runtests.jl

+    """))
+    for typ in reader
+        if typ == EzXML.READER_ELEMENT
+            if EzXML.nodename(reader) == "a"


Please do not prefix exported names so that we can check names are surely exported.

bicycle1885 · 2018-08-30T11:10:58Z

src/streamreader.jl

+    return attrs
+end
+
+function _getattr(reader::StreamReader, name::AbstractString)


I would name it attribute_ptr since I don't start function names with underscore.

src/streamreader.jl

bicycle1885 · 2018-08-31T02:24:48Z

test/runtests.jl

+                @test hasnodeattributes(reader)
+                @test countattributes(reader) == 2
+                @test nodeattributes(reader) == Dict{String,String}("attr1"=>"", "attr2"=>"This is cool")
+                @test reader[0] == ""


I think 1-based indexing is less confusing to most Julians. 0-based indexing is an implementation detail of libxml2 and it shouldn't be exposed to users.

bicycle1885 · 2018-08-31T02:27:47Z

test/runtests.jl

+                        @test nodevalue(attr) == "This is cool"
+                    end
+                end
+            elseif EzXML.nodename(reader) == "b"


EzXML. is unnecessary here.

bicycle1885 · 2018-08-31T02:36:45Z

src/streamreader.jl

@@ -82,6 +84,48 @@ const READER_END_ELEMENT            = ReaderType(15)
 const READER_END_ENTITY             = ReaderType(16)
 const READER_XML_DECLARATION        = ReaderType(17)

+function Base.convert(::Type{Symbol},x::ReaderType)


Do we really need this conversion method? I'm not sure when we want this.

bicycle1885 · 2018-08-31T02:39:43Z

src/streamreader.jl

+mutable struct AttributeReader
+    reader::StreamReader
+
+    function AttributeReader(reader::EzXML.StreamReader)


You still leave unnecessary EzXML. prefixes in several places.

bicycle1885 · 2018-08-31T11:17:08Z

test/runtests.jl

        @test t == i
        @test occursin(r"READER_[A-Z_]+$", repr(t))
        @test string(t) == string(i)
        @test convert(EzXML.ReaderType, t) === t
    end
    @test_throws AssertionError repr(convert(EzXML.ReaderType, -1))
    @test_throws AssertionError repr(convert(EzXML.ReaderType, 18))
+    @test_throws AssertionError convert(Symbol, EzXML.ReaderType(18))


I think this won't pass. Also, please delete unnecessary blank lines above.

bicycle1885 · 2018-09-01T07:11:38Z

The CI failures on 32-bit Windows would be noise. Thank you for your great pull request!

programmeroftheeve added 3 commits August 28, 2018 16:40

Add StreamReader Attribute Functions

3a25101

Add Tests for Attribute StreamReader Functions

2ec477f

Add some documentation for attribute StreamReader functions

dc412f2

programmeroftheeve added 3 commits August 28, 2018 21:05

Add more tests for CodeCov

13cdfe5

Fix wrong ccall function name

18b2085

Add more tests!!!

2dff7ec

bicycle1885 requested changes Aug 30, 2018

View reviewed changes

programmeroftheeve added 12 commits August 30, 2018 09:33

Update hash function to use the 2-arg version

4a75d6e

Rename _getattr to attribute_ptr

c242d3a

Throw is node does not have a value

cc9bf97

Throw error if r < 0

65ef777

Fix AttributeReader eltype and constructor

ec2d6de

Fixed docstring to use correct function name and grammar

191a6f3

Rename nodeattributecount

ed48b06

Fix exported functions

dba0158

Fix Tests

9be0187

Change error message to be more inline with other messages

26ad786

Check return value of moving back to the element

57f3f9b

Only move back to element if node is an attribute

296fe09

programmeroftheeve closed this Aug 30, 2018

programmeroftheeve reopened this Aug 30, 2018

bicycle1885 reviewed Aug 31, 2018

View reviewed changes

programmeroftheeve added 4 commits August 31, 2018 06:57

Remove unnessary EzXML from code

439a231

Use 1 based indexing

53b1f60

Remove conversion of ReaderType to Symbol

664b738

Remove unnessary EzXML from code

ab4cae3

programmeroftheeve added 2 commits August 31, 2018 07:06

Don't crash on using nodecontent functions on attribute nodes

b90afb7

Missed a test

dacef71

bicycle1885 reviewed Aug 31, 2018

View reviewed changes

Clean up some blank lines

360dfb6

bicycle1885 approved these changes Sep 1, 2018

View reviewed changes

bicycle1885 merged commit 4e3f059 into JuliaIO:master Sep 1, 2018

programmeroftheeve deleted the attr branch September 6, 2018 20:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Attribute Functions for the SteamReader #90

Add Attribute Functions for the SteamReader #90

programmeroftheeve commented Aug 28, 2018 •

edited

Loading

codecov-io commented Aug 28, 2018 •

edited

Loading

bicycle1885 left a comment

bicycle1885 Aug 30, 2018

bicycle1885 Aug 30, 2018

bicycle1885 Aug 30, 2018

programmeroftheeve Aug 30, 2018

bicycle1885 Aug 31, 2018

programmeroftheeve Aug 31, 2018

bicycle1885 Aug 30, 2018

bicycle1885 Aug 30, 2018

bicycle1885 Aug 30, 2018

bicycle1885 Aug 30, 2018

bicycle1885 Aug 30, 2018

bicycle1885 Aug 30, 2018

bicycle1885 Aug 31, 2018

bicycle1885 Aug 31, 2018

bicycle1885 Aug 31, 2018

bicycle1885 Aug 31, 2018

bicycle1885 Aug 31, 2018

bicycle1885 commented Sep 1, 2018

Add Attribute Functions for the SteamReader #90

Add Attribute Functions for the SteamReader #90

Conversation

programmeroftheeve commented Aug 28, 2018 • edited Loading

codecov-io commented Aug 28, 2018 • edited Loading

Codecov Report

bicycle1885 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bicycle1885 commented Sep 1, 2018

programmeroftheeve commented Aug 28, 2018 •

edited

Loading

codecov-io commented Aug 28, 2018 •

edited

Loading