Skip to content

Conversation

TimG1964
Copy link
Contributor

I've added a simple function to recursively copy a node and all its children. I've added some tests for this, too.

The text on the Readme page currently says:

  • Node is an immutable type. However, you can easily create a copy with one or more field values changed by using the Node(::Node; kw...) constructor where kw are the fields you want to change. For example:
node = XML.Element("tag", XML.Text("child"))

simple_value(node)
# "child"

node2 = Node(node, children=XML.Text("changed"))

simple_value(node2)
# "changed"

This seems to be a bit out of date. The Node() function seems to treat arguments as new children and keywords as new/changed attributes. The example presented now seems to create an attribute called "children" rather than changing the child node. I've suggested some changes to the Readme to reflect this.

(Hope this goes OK this time!)

TimG1964 added a commit to TimG1964/XML.jl that referenced this pull request Jun 1, 2025
This gc call was added in Windows only, apparently in response to issue JuliaComputing#42.

The consequence was that `writexlsx()` was much slower, with 97% of time spent on gc (in my use case).

Deleting this line speeds up `writexlsx()` forty-fold and XLSX still passes all tests.

Not sure what, if any, downsides there might be in other use cases.
@TimG1964
Copy link
Contributor Author

TimG1964 commented Jun 4, 2025

Reflecting on this discussion on Discourse, maybe this new function shouldn't overload Base.copy. Perhaps XML.copynode would be a better name.

@joshday
Copy link
Member

joshday commented Jun 6, 2025

Hmm, I don't think I like having an XML.copynode, creating three different ways (including copy and deepcopy) to create a copy.

With all the concerns I've seen written about deepcopy, I've never experienced any unwanted effects from it.

@TimG1964
Copy link
Contributor Author

TimG1964 commented Jun 6, 2025

OK, I understand. Just to note, though, that deepcopy is more expensive than this proposed function:

julia> f=XLSX.openxlsx(raw"C:\Users\tim\OneDrive\Documents\Julia\XLSX\XLSX.jl\data\customXml.xlsx")
XLSXFile("C:\Users\tim\OneDrive\Documents\Julia\XLSX\XLSX.jl\data\customXml.xlsx") containing 2 Worksheets
            sheetname size          range        
-------------------------------------------------
              Mock-up 116x11        A1:K116      
     Document History 17x3          A1:C17

julia> @time begin; for i=1:10; deepcopy(f.data["xl/workbook.xml"]); end; end
  0.000710 seconds (14.20 k allocations: 765.736 KiB)

julia> @time begin; for i=1:10; XLSX.copynode(f.data["xl/workbook.xml"]); end; end
  0.000143 seconds (3.77 k allocations: 193.594 KiB)

Difference may only be marginal for most use cases, though.

@TimG1964
Copy link
Contributor Author

TimG1964 commented Jun 9, 2025

I captured the issue of out of date text on the Readme page in #44.

@TimG1964 TimG1964 closed this Jun 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants