Skip to content

Conversation

TimG1964
Copy link
Contributor

@TimG1964 TimG1964 commented Mar 22, 2025

I've added a number of new functionalities of most use when writing xlsx files:

This pull request rolls up all the changes proposed in #286, #287 and #288, which can probably be closed.

@felipenoris - altogether, I've made some quite big changes. If you'd like, I can post on Discourse describing them all and ask for others to try them out. This should give me an opportunity to fix any bugs that show themselves before you make a new release. Does that sound OK?

@TimG1964
Copy link
Contributor Author

TimG1964 commented Mar 22, 2025

Not sure about this. My previous PR succeeded and I didn't change the CI.
All tests pass locally.

TimG1964 added a commit to TimG1964/XLSX.jl that referenced this pull request Mar 23, 2025
@TimG1964
Copy link
Contributor Author

Hi again @felipenoris! Forgive me for still inching my way up the GitHub learning curve. I realise now that my suggestion isn't the best approach. I didn't realise that I could share my fork directly. I think this will be better. Then, if any one is interested, they can file bugs on my fork. I can (try to) resolve and then make another pull request on your package after that. Sorry for seeming to complicate matters before. I hope this new suggestion is better. If you have a better suggestion still, please let me know. Otherwise my coaches are google and AI, and learning is limited by my own imagination! Thanks for your patience!

- '1.7'
- '1.8'
- '1.9'
- '1' # automatically expands to the latest stable 1.x release of Julia
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add - '1.10' (or equivalently - 'lts') to the matrix

@nhz2
Copy link
Contributor

nhz2 commented Mar 23, 2025

In terms of releasing a new version of XLSX.jl, we should wait for the next release of XML.jl to get bug fixes JuliaComputing/XML.jl#37 and JuliaComputing/XML.jl#32 .
As you can see in the downstream tests in https://github.com/JuliaComputing/XML.jl/actions/runs/13790200632/job/38567809829 the bug fix in JuliaComputing/XML.jl#37 breaks the development version of this package.

@TimG1964
Copy link
Contributor Author

TimG1964 commented Mar 23, 2025

In terms of releasing a new version of XLSX.jl, we should wait for the next release of XML.jl to get bug fixes JuliaComputing/XML.jl#37 and JuliaComputing/XML.jl#32 .

Thanks, @nhz2. That makes sense.

As you can see in the downstream tests in https://github.com/JuliaComputing/XML.jl/actions/runs/13790200632/job/38567809829 the bug fix in JuliaComputing/XML.jl#37 breaks the development version of this package.

I was not aware of this. How does one test XLSX.jl against downstream tests of another application?

In terms of the fix, it looks pretty simple on the face of it. I know looks can be deceptive and I don't know how to test it myself, but simply changing Worksheet.jl:40 from

@assert XML.depth(sheet_row) == 1 "Malformed Worksheet \"$(ws.name)\": unexpected node depth for dimension node: $(XML.depth(sheet_row))."

to

@assert XML.depth(sheet_row) == 1 "Malformed Worksheet \"$name\": unexpected node depth for dimension node: $(XML.depth(sheet_row))."

looks like it should do it. I can't test the node depth because of the bug you identified in XML.jl, but the test was also ==1 when XLSX relied on EzXML.jl, so I'm still assuming it is right.

I've included the above fix in the branch Bug-fixing-post-#289

@nhz2
Copy link
Contributor

nhz2 commented Mar 24, 2025

To test XLSX with an unreleased version of another package, I am using Pkg.develop to add both packages to a temporary environment, and then testing XLSX.
Here is the code. You would need to adjust path to the location where the local versions of the packages are.

using Pkg
Pkg.Registry.update()
Pkg.activate(;temp=true)
Pkg.develop([
    PackageSpec(path="/home/nathan/github/XLSX.jl"),
    PackageSpec(path="/home/nathan/github/XML.jl"),
])
Pkg.update()
Pkg.test("XLSX")

You should see something like:

     Testing XLSX
...
      Status `/tmp/jl_0E8Pxi/Manifest.toml`
...
  [fdbf4ff8] XLSX v0.10.5-dev `~/github/XLSX.jl`
  [72c71f33] XML v0.3.4 `~/github/XML.jl`

if things are working and Pkg.test is using the local versions of the packages.

I think the fix is to change the == 1 to == 2 since from what I can tell EzXML depth starts at 0 whereas XML starts at 1.

@TimG1964
Copy link
Contributor Author

TimG1964 commented Mar 24, 2025

Oh, that makes sense!

I'll give it a go.

EDIT. Yes. If I change ws.name to name and change the test to ==2 then it works as you suggest with the devd XML package. I just ran the tests locally rather than on GitHub.

There is another test for node depth in stream.jl and making the same change from ==1 to ==2 works there as well.

Thanks!

Copy link

codecov bot commented Apr 21, 2025

Codecov Report

Attention: Patch coverage is 63.57724% with 224 lines in your changes missing coverage. Please review.

Project coverage is 85.49%. Comparing base (f4767c4) to head (746c051).
Report is 14 commits behind head on master.

Files with missing lines Patch % Lines
src/cellformats.jl 43.58% 66 Missing ⚠️
src/worksheet.jl 25.00% 66 Missing ⚠️
src/cellref.jl 65.62% 55 Missing ⚠️
src/workbook.jl 75.00% 23 Missing ⚠️
src/read.jl 82.92% 7 Missing ⚠️
src/write.jl 92.64% 5 Missing ⚠️
src/table.jl 97.67% 1 Missing ⚠️
src/types.jl 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #289      +/-   ##
==========================================
- Coverage   95.02%   85.49%   -9.53%     
==========================================
  Files          15       16       +1     
  Lines        2009     3462    +1453     
==========================================
+ Hits         1909     2960    +1051     
- Misses        100      502     +402     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@TimG1964
Copy link
Contributor Author

I wasn't aware of these codecov tests but I obviously should have been! They don't run when I make a PR.

I've downloaded Coverage.jl and will try to use it to resolve the issues and then make a new PR.

@felipenoris
Copy link
Owner

felipenoris commented Apr 22, 2025

@TimG1964 , no need to worry too much about codecov. But it is a good idea to cover some of the new code with some tests if possible.

@TimG1964
Copy link
Contributor Author

Will close this now and make a new PR when I've added a bunch more tests to improve coverage!

@TimG1964 TimG1964 closed this Apr 23, 2025
TimG1964 added a commit to TimG1964/XLSX.jl that referenced this pull request Apr 24, 2025
@TimG1964 TimG1964 deleted the Add-Row-Ranges-to-formatting branch June 5, 2025 08:49
felipenoris pushed a commit that referenced this pull request Sep 12, 2025
* Add `newxlsx()` and `opentemplate()` and `CellRef` to docs

* A few typos

* Add row ranges to formatting functions.

* Update CI

* Add `normalizenames` keyword to `XLSX.readtable` (#260)

* Add three functions for merged cells (no tests yet)
to address #241

* Functions to add Defined Names (more work to do)

* Add more support for non-contiguous ranges in defined names.

* Add fix to #239 in writetable!() function

* More updates to support non-contiguous cell ramges

* Preparing to write definedNames in `xlsxwrite()`

* Bug fixing new code with existing tests

* Support `getdata()` for row ranges

* Handle apostrophes in sheet names in `definedNames`

* Write new `definedNames` out to a new XLSXFile

* Handle apostrophes correctly in sheetname part of `definedNames`

* Minor row range fixes

* Begin adding some tests

* Add new functions to API docs

* Add more tests

* Minor changes to docstrings...

* Missing line in CI

* Try again. :-(

* Bump to julia 1.8 and above

* New branch for bug fixing post PR #289

* Add 'lts' to CI per @Eben60 review

* Fix to bug highlighted when `Node.depth` bug in XML
was itself fixed (as yet untested).

* Previous fix but now with actual change!!

* Update CI to include `aarch64` for MacOS-latest

* Include versions in CL for MacOS aarch64

* Now with all julia versions specified for aarch64, too.
(is this right???)

* Add test for `setOutsideBorder`

* Fix the two node depth tests to `XML.depth==2`
(XML uses 1-based counting whereas EzXML used 0-based)

* Begin to introduce support for a few named colors

* Add support for named colors from Colors.jl

* Revert to XML.escape with XML.jl 0.3.5

* Integrate `setOutsideBorder` into `setBorder`

* Minor tweaks!

* Fix `getBorder()` for diagonal borders

* Add examples of named colors in docstrings

* Update tests for changes to getBorder
Fix behaviour of `setUniformFormat()`

* Replace `@assert` with `throw()` (#190)

* Row and/or column indexing with vectors of Ints (#276)

* Address #258 as proposed...

* Address issue #147
Also remove dependency of `getcellrange()` functions on `eachrow()`

* Update tests for changes to `getcellranges`

* Change `error()` to `throw(XLSXError())`

* Extend indexing options for `setdata!()`

* Extend indexing for `setAttribute` family of cell formatting functions.

* Add to docstrings.

* Fix indexing issues

* Split cellformat-helpers.jl into separate file

* Tweak docstrings...

* Add some tests for indexing `setAttribute` functions

* Fix `setUniformAttribute` call error

* A few more tests...

* Add another set of tests.

* More tests

* Test valid range names

* Fix logic error.

* Include changes proposed in #287
Thought I'd done this a while ago (my bad!)

* Single cell range of one empty cell

* Remove most string interpolation outside `throw()`

* Clarify use of `definedNames`

* Typo!

* Address issue #88
Allow indexing a `sheetrow` or `tablerow` with either a `UnitRange`
or a vector of integers.

* Updated to fix issue in #292

* Updated with tests for #292

* Support merging of cells (#241 & #184)

* Typo!

* Improved handling of non-contiguous ranges

* Couple of overlooked changes!

* Handle non-contiguous ranges for `setUniformAttribute()` functions

* Small change to docs.

* Fix behavior of `getFormat` when no keywords given.

* Add `StepRange` to indexing options.

* Detect Excel `.xltx` template files and throw (#293).
Also rationalise `cellformat-helpers`

* Address #155 and #52

* Filename typo!

* Try again!

* Take out recalcitrant file.

* ... and replace it!

* Minor typos

* Add ability to delete a worksheet (#80)

* Add tests for `deletesheet()`

* Revisions to docs

* Begin to add some dynamic conditional formatting

* Add some docs for `colorScale` conditional formats

* Adding tests to improve code coverage

* A bunch more tests for coverage.

* Continue adding tests for coverage...

* Now over 90% code-cov according to Coverage.jl!

* Small changes

* Additions to formatting guide

* Finish first version of `colorscale` conditional formatting

* Tweaks

* Remove stray `=` following conflict resolution

* Typo in file name in runtests.jl

* Correct type in runtests.jl

* Begin to add `:cell` type conditional formats

* `:cell` type now basically working

* Further work on `:cell` type conditional formats

* All conditional formats except `dataBars` and `iconSets`.

Still need to add tests and examples.

* Minor revisions to docs.

* Unify priority to allow overlapping conditional formats

* Add tp formatting.md

* More documentation.

* More minor changes to docstrings

* More docs!

* Trying to finish docs!

* Start adding some tests

* Add more tests for conditional formats

* Escape formulas properly

* Change compats but I don't fully umderstand the compat system!

* Changed for Random as well as Distributions!

* Add `expression` to the list of supported types for conditional formats

* Add `iconSets` (no tests yet)

* Add tests for `:iconSet` conditional formats

* Fix compats on UUIDs

* Tweaks to docs

* Add `dataBar` conditional formatting (with tests)

* Update `ci.yml` to match xlsx master

* Reinstate 'lts'

* Minor tweaks to docs

* take timer out of tests

* Tweaks and tidy-up

* Update README.md

* Eliminate statements like `XML.parse(XML.Node, XML.write(node))`

* Remove some dead code.

* Remove some dead code.

* Use new `copynode(::Node)` instead of `deepcopy()`

* Pack conditional format keywords into a Dict to
improve compilation time.
Reduce number of xalls to `find_all_nodes()` to
improve performance.

* Rationalise some function calls.

* Generate a new uuid when adding a new worksheets.

* Add `copysheet!` with docs and tests. Also include
code from #300 and #302

* Restructured docs a bit

* Restructure `eachrow` iterator.
Separate stream interator from cache iterator

* Replace `eachrow` with direct cache access (sometimes).

* Add minor test to worksheet `dimension`

* Minor tidy-up!

* Fix #250 and add tests for same.

* Add mmap support for files too big for memory.
Also add `PrecompileTools` (#277)

* Relax compat limits on `Mmap.jl`

* Work around XML.jl issue 43 (JuliaComputing/XML.jl#43)
and allow cells containing only spaces retain their value.

* Improve work around issue 43 in XML.jl
(JuliaComputing/XML.jl#43)

Can now write cells containing only whitespage characters or with leading or trailing whitespace.
Cannot see these things in cells read in from existing Excel files - XML removes leading whitespace
even if `xml:space="preserve"` is specified.
Thus a cell containing "    " will be read in as missing. A cell containing "  hello" will become "hello".

* Fix failing line!

* Properly unescape strings in `getdata`

* Add a `savexlsx` function with docs and tests.

* Address #243

* Remove need to write to a string on reading (again)

* Generalise `readto` to support StructArray as sink
(mirrors CSV.jl)

* `readdf` -> `readto` in tests

* Update API documentation to `readto`

* And again in docs :-(

* Minor performance changes

* Add streaming based on `fileArray`.

Based on code first suggested by Fabian Gans here:
https://discourse.julialang.org/t/struggling-to-use-mmap-with-ziparchives/129839/17
and developed for ZipArchives by Nathan Zimmerberg here:
https://discourse.julialang.org/t/struggling-to-use-mmap-with-ziparchives/129839/21

* Small changes, mainly comments.

* Tidy-up

* comments

* Minor changes to docs.

* Add tests for when #303 is fixed (currently commented)
Depends on fix of issue 43 in XML.jl: JuliaComputing/XML.jl#43

* Escape formulae on write, extending #302.

* Revise [compats]

* Restore eager filling of cache only in write mode

* Rudimentary fix for #159 (feedback welcome!)

* Begin to understand #159

* Try to prevent orphaned FormulaReference cells

* Finish resolving #159 - I hope! No tests yet.

* Now with added tests for #159

* Manage cell references during `deletedheet!`

* Fix #312 and update to XML.jl 0.3.6 to fix #303

* Separate <row> and <sst> elements for speed-up

* Change noDim test to be less brittle

* Minor optimizations.

* Multi-threaded reading of <row> XML elements

* Reading sst table now multi-threaded, too.

* Reading files (in write mode) now multi-threaded

* Small tweaks

* Remove some redundant lines

* Add simple test for stream iterator.
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.

4 participants