Maintainability and bug fixing

Project.toml

@@ -1,7 +1,7 @@
 name = "XLSX"
 uuid = "fdbf4ff8-1666-58a4-91e7-1b58723a45e0"
 license = "MIT"
-version = "0.13.1"
+version = "0.13.2"
 authors = ["Felipe Noronha <felipenoris@gmail.com>"]
 repo = "https://github.com/juliadata/XLSX.jl.git"
 

bench/README.md

@@ -112,6 +112,30 @@ It needs no fixtures and no BenchmarkTools, so it is a quick repro for read-path
 
     julia --project=envs/dev --check-bounds=yes worksheet_read.jl
 
+### Maintainability check
+
+`maintainability.jl` measures maintainability signals over `src/` and compares them
+with `maintainability_baseline.toml`, exiting with status 1 if any has risen. Lower
+is better for every metric:
+
+- `duplicated_lines` — lines in an 8-line block that also occurs elsewhere. A
+  method defined in short form has its own name blanked first, so a family of
+  methods copied from one function to the next counts as duplicated
+- `bare_catch`, `something_wrong`, `asserts` — error handling that hides or fails
+  to explain errors
+- `commented_out_code` — comments holding statements
+- `inline_guards` — writability or cache checks written out rather than through
+  `_require_writable`/`_require_cache`
+- `functions_over_100`, `largest_file_lines` — size
+
+It needs only the standard library and takes a few seconds:
+
+    julia bench/maintainability.jl            # report; exit 1 if any metric rose
+    julia bench/maintainability.jl --update   # report and store the new baseline
+
+Run it before committing. When a metric falls, store the new baseline with
+`--update`, so it can't creep back up.
+
 ### Fixture descriptions
 
 Written through XLSX.jl by `generate_fixtures.jl` (pretty-printed XML):

bench/maintainability.jl

@@ -0,0 +1,167 @@
+# maintainability.jl
+# Measures a few maintainability signals over src/ and compares them with the stored
+# baseline (maintainability_baseline.toml), so a rise in copy-paste or error masking
+# shows up as a number rather than in review. Lower is better for every metric.
+#
+# Usage: julia bench/maintainability.jl            # report; exit 1 if any metric rose
+#        julia bench/maintainability.jl --update   # report and store the new baseline
+#
+# Needs only the standard library, so no environment is required.
+
+using TOML
+
+const SRC      = normpath(joinpath(@__DIR__, "..", "src"))
+const BASELINE = joinpath(@__DIR__, "maintainability_baseline.toml")
+const WINDOW   = 8   # lines in a duplicated block
+
+source_files() = sort!([joinpath(r, f) for (r, _, fs) in walkdir(SRC) for f in fs if endswith(f, ".jl")])
+rel(p) = replace(relpath(p, SRC), '\\' => '/')
+
+# Lines as compared for duplication: trimmed, string and number literals blanked, and
+# blank lines, comments and lone closers dropped. On a line defining a method in short
+# form, `name(...) = ...`, `name` is blanked wherever it appears, so a family of methods
+# copied from one function to the next counts as duplicated even though each names its
+# own function. Returns (line number, text) pairs.
+function code_lines(lines)
+    out = Tuple{Int,String}[]
+    for (i, l) in enumerate(lines)
+        s = replace(strip(l), r"\"[^\"]*\"" => "S", r"\b\d+(\.\d+)?\b" => "N")
+        m = match(r"^([A-Za-z_][A-Za-z0-9_]*!?)\(.*\)\s*(::[^=]+)?=\s", s)
+        if m !== nothing
+            s = replace(s, Regex("\\b" * replace(m[1], "!" => "\\!") * "(?![A-Za-z0-9_])") => "F")
+        end
+        (isempty(s) || startswith(s, "#") || s in ("end", "else", ")", "]", "\"\"\"")) && continue
+        push!(out, (i, s))
+    end
+    return out
+end
+
+# Lines lying in a block of `WINDOW` consecutive code lines that also occurs elsewhere.
+function duplicated_lines(files)
+    seen = Dict{UInt,Vector{Tuple{String,Int}}}()
+    for f in files
+        cl = code_lines(readlines(f))
+        for k in 1:length(cl)-WINDOW+1
+            h = hash(join(last.(cl[k:k+WINDOW-1]), "\n"))
+            push!(get!(seen, h, Tuple{String,Int}[]), (f, k))
+        end
+    end
+    dup = Dict{String,Set{Int}}()
+    for locs in values(seen)
+        length(locs) > 1 || continue
+        for (f, k) in locs
+            cl = code_lines(readlines(f))
+            union!(get!(dup, f, Set{Int}()), first.(cl[k:k+WINDOW-1]))
+        end
+    end
+    return dup
+end
+
+# Top-level and one-level-nested functions written `function name ... end`, with lengths.
+function long_functions(files; limit=100)
+    long = Tuple{String,String,Int}[]
+    for f in files
+        L = readlines(f)
+        for (i, l) in enumerate(L)
+            m = match(r"^(    )?function ([^\s(]+)\(", l)   # `(` so docstring prose doesn't match
+            (m === nothing || endswith(rstrip(l), " end")) && continue
+            closer = m[1] === nothing ? "end" : "    end"
+            j = findnext(x -> rstrip(x) == closer, L, i + 1)
+            j === nothing || j - i <= limit || push!(long, (rel(f), m[2], j - i))
+        end
+    end
+    return sort!(long, by=x -> -x[3])
+end
+
+count_matching(files, re) = sum(count(l -> occursin(re, l), readlines(f)) for f in files)
+
+# Commented-out code: a comment holding a statement, with no space after `#` or with
+# several (`#foo(x)`, `#    x = 1`). Prose comments have exactly one space, so don't match.
+const COMMENTED_CODE = r"^\s*#(?![=!])(?:\S|\s{2,})\s*(?:[A-Za-z_.!]+\(.*\)\s*$|[A-Za-z_.\[\]\"]+ *= [^=]|@[a-z]+\b|for [a-z_(]|if |elseif |else\b|end\b|return\b|throw\()"
+
+# Writability or cache checks that throw, written inline rather than with
+# `_require_writable`/`_require_cache`: the flag tested on a line that throws, or on
+# the line before one.
+function inline_guards(files)
+    flag = r"\.(is_writable|use_cache_for_sheet_data)\b|is_writable\("
+    n = 0
+    for f in files
+        L = readlines(f)
+        for (i, l) in enumerate(L)
+            occursin(flag, l) || continue
+            occursin("throw(", l) || (i < length(L) && occursin(r"^\s*throw\(", L[i+1])) || continue
+            # the helpers' own checks
+            (occursin("function _require_", l) || (i > 1 && occursin("function _require_", L[i-1]))) && continue
+            n += 1
+        end
+    end
+    return n
+end
+
+function measure()
+    files = source_files()
+    total = sum(length(readlines(f)) for f in files)
+    dup = duplicated_lines(files)
+    ndup = sum(length, values(dup); init=0)
+    long = long_functions(files)
+    sizes = sort!([(rel(f), length(readlines(f))) for f in files], by=x -> -x[2])
+    metrics = Dict{String,Int}(
+        "duplicated_lines"   => ndup,
+        "bare_catch"         => count_matching(files, r"^\s*catch\s*$"),
+        "something_wrong"    => count_matching(files, r"Something wrong"),
+        "asserts"            => count_matching(files, r"@assert\b"),
+        "commented_out_code" => count_matching(files, COMMENTED_CODE),
+        "inline_guards"      => inline_guards(files),
+        "functions_over_100" => length(long),
+        "largest_file_lines" => first(sizes)[2],
+    )
+    return metrics, (; files, total, dup, long, sizes)
+end
+
+function report(metrics, detail, baseline)
+    println("src/: ", length(detail.files), " files, ", detail.total, " lines\n")
+    rose = String[]
+    println(rpad("metric", 22), lpad("now", 7), lpad("baseline", 10))
+    for k in sort(collect(keys(metrics)))
+        b = isnothing(baseline) ? nothing : get(baseline, k, nothing)
+        flag = (!isnothing(b) && metrics[k] > b) ? "  ▲ rose" : ""
+        isempty(flag) || push!(rose, k)
+        println(rpad(k, 22), lpad(metrics[k], 7), lpad(isnothing(b) ? "-" : string(b), 10), flag)
+    end
+    pct = round(100 * metrics["duplicated_lines"] / detail.total, digits=1)
+    println("\nduplicated lines are $pct% of src/ ($(WINDOW)-line blocks)")
+    if !isempty(detail.dup)
+        println("most duplicated files:")
+        for (f, s) in first(sort(collect(detail.dup), by=x -> -length(x[2])), 5)
+            println("  ", rpad(rel(f), 40), length(s))
+        end
+    end
+    if !isempty(detail.long)
+        println("functions over 100 lines:")
+        for (f, name, n) in detail.long
+            println("  ", rpad("$f  $name", 60), n)
+        end
+    end
+    println("largest files:")
+    for (f, n) in first(detail.sizes, 5)
+        println("  ", rpad(f, 40), n)
+    end
+    return rose
+end
+
+metrics, detail = measure()
+baseline = isfile(BASELINE) ? Dict{String,Int}(k => v for (k, v) in TOML.parsefile(BASELINE)) : nothing
+rose = report(metrics, detail, baseline)
+
+if "--update" in ARGS
+    open(BASELINE, "w") do io
+        println(io, "# Written by bench/maintainability.jl --update. Lower is better for every metric.")
+        TOML.print(io, metrics; sorted=true)
+    end
+    println("\nbaseline updated: ", BASELINE)
+elseif isnothing(baseline)
+    println("\nno baseline yet: run with --update to store one")
+elseif !isempty(rose)
+    println("\nrose since the baseline: ", join(rose, ", "))
+    exit(1)
+end

bench/maintainability_baseline.toml

@@ -0,0 +1,9 @@
+# Written by bench/maintainability.jl --update. Lower is better for every metric.
+asserts = 3
+bare_catch = 0
+commented_out_code = 0
+duplicated_lines = 1248
+functions_over_100 = 5
+inline_guards = 0
+largest_file_lines = 2973
+something_wrong = 0

docs/src/api/chartElements.md

@@ -13,8 +13,10 @@ getAxisPartner
 getAxisShapeProps
 getAxisTextProps
 getAxisGridlines
+getAxisTitle
 getAxisTitleText
 getAxisTitleRef
+getAxisTitleRange
 getAxisTitleTextProps
 setAxisTitleText
 setAxisTitleTextProp

docs/src/api/chartTypes.md

@@ -33,7 +33,7 @@ FormatSite
 Effective
 ```
 
-## Colours and DrawingML
+## Colors and DrawingML
 
 ```@docs
 SchemeColor

docs/src/api/charts.md

@@ -18,6 +18,36 @@ The names released before the sub-module existed — `getCharts`, `getChart`,
 XLSX.Charts
 ```
 
+## Conventions
+
+The setters share their argument conventions, so the docstrings below don't
+repeat them.
+
+| argument     | meaning                                                                                   |
+|:-------------|:------------------------------------------------------------------------------------------|
+| `c`          | a chart, from [`getCharts`](@ref), [`getChart`](@ref), [`addChart`](@ref) or [`addChartEx`](@ref) |
+| `i`          | a series' position, counting from 1, in the order [`getChartSeries`](@ref) returns them   |
+| `point`      | a data point's position in its series, counting from 1                                    |
+| `ax`         | a [`ChartAxis`](@ref), from [`getChartAxes`](@ref) or [`getChartAxis`](@ref)              |
+| `g`          | a [`ChartGroup`](@ref), from [`getChartGroups`](@ref)                                      |
+| `id`         | on a `ChartEx`, an axis's `id`, from [`getChartAxisIds`](@ref) — not a position           |
+| `pareto`     | on a `ChartEx`, `true` to act on a Pareto chart's line rather than its bars               |
+
+Every setter returns `c`. Most take `:inherit` in place of a value, to remove
+the setting so that it is inherited from the chart style or from the level
+above; each docstring says where it does. That is different from `:none`, which
+some setters take to mean "draw nothing".
+
+Three vocabularies are shared by many setters, and each is listed in full once:
+
+- **Color values** — at [`setSeriesFill`](@ref): color names, hex strings,
+  `Colors.Colorant`s, theme colors such as `"accent2"`, and
+  [`SchemeColor`](@ref)s.
+- **Line keywords** (`color`, `width`, `dash`, `cap`, `compound`, `join`,
+  `miterLimit`) — at [`setSeriesLine`](@ref).
+- **Text-property fields** (`:size`, `:bold`, `:fill`, `:font`, …) — at
+  [`setChartTitleTextProp`](@ref).
+
 ## Finding and reading charts
 
 ```@docs
@@ -37,7 +67,9 @@ getChartTypes
 getChartGroups
 getChartLegend
 getLegendPos
+setLegendPos
 getLegendOverlay
+setLegendOverlay
 getLegendShapeProps
 getLegendTextProps
 setLegendTextProp

docs/src/api/data.md

@@ -4,6 +4,7 @@
 
 ```@docs
 XLSX.CellRef
+XLSX.SheetCellRef
 XLSX.row_number
 XLSX.column_number
 XLSX.eachrow

docs/src/api/files.md

@@ -5,6 +5,7 @@
 ```@docs
 XLSX.XLSXFile
 XLSX.XLSXFile(::Any)
+XLSX.Workbook
 XLSX.readxlsx
 XLSX.readto
 XLSX.openxlsx

docs/src/api/tables.md

@@ -8,6 +8,7 @@ XLSX.gettransposedtable
 XLSX.readtransposedtable
 XLSX.writetable!
 XLSX.Table
+XLSX.TableStyleInfo
 XLSX.tables
 XLSX.table
 XLSX.addtable!

docs/src/charts/chartLimitations.md

@@ -31,7 +31,7 @@ for the `c:` kinds, their cached data. The restriction is on creating them.
 A combo chart is also the one case [`addSeries`](@ref) cannot extend, since
 there is no single group to add the series to.
 
-## Style and colour parts are not read
+## Style and color parts are not read
 
 Excel keeps much of a chart's appearance in two parts beside the chart itself,
 `style1.xml` and `colors1.xml`. Neither is read into the formatting cascade, so
@@ -100,7 +100,7 @@ A marker is a property of a line, scatter or radar series. Setting one on a bar
 or pie series throws, because the schema gives `c:barSer` no `c:marker` child.
 The same applies to the options [`addSeries`](@ref) takes: `smooth` is for line
 and scatter series, `line` for scatter, and `color` has no meaning on a pie or
-doughnut chart, whose points are coloured individually.
+doughnut chart, whose points are colored individually.
 
 ## Resolving text properties
 

docs/src/charts/chartSheets.md

@@ -16,7 +16,7 @@ julia> using XLSX, XLSX.Charts
 
 Copying, renaming and deleting sheets are core operations, but a sheet holding a
 chart carries more than cells: a drawing part, one chart part per chart, and the
-style and colour parts beside it. This page is what happens to those.
+style and color parts beside it. This page is what happens to those.
 
 ## Copying a sheet
 
@@ -60,7 +60,7 @@ Cached values are untouched, since the data hasn't changed.
 ## Deleting a sheet
 
 [`XLSX.deletesheet!`](@ref) removes the charts anchored to the sheet, along with
-their drawing, style and colour parts and the content-type entries for all of
+their drawing, style and color parts and the content-type entries for all of
 them:
 
 ```julia

docs/src/charts/creatingCharts.md

@@ -57,7 +57,7 @@ schema, where the data block is required.
 
 Each comes from a chart part Excel itself wrote, kept verbatim in the package,
 so a created chart is what Excel would have produced for that kind — including
-its style and colour parts. Kinds with no template yet throw rather than
+its style and color parts. Kinds with no template yet throw rather than
 approximate; see [Limitations](chartLimitations.md).
 
 ## A chart on its own sheet
@@ -145,17 +145,17 @@ julia> addSeries(c, "C2:C5"; categories = "A2:A5", name = "2025", color = "FF000
 
 | Keyword | Applies to | Takes |
 | --- | --- | --- |
-| `color` | everything but pie and doughnut | a colour name, hex, a `Colorant`, a [`SchemeColor`](@ref), or a scheme name such as `"accent4"` |
+| `color` | everything but pie and doughnut | a color name, hex, a `Colorant`, a [`SchemeColor`](@ref), or a scheme name such as `"accent4"` |
 | `markers` | line, lineMarkers, radar, scatter | a symbol such as `:circle`, or `false` for none |
 | `smooth` | line, lineMarkers, scatter | `true` or `false` — whether the line is curved |
 | `line` | scatter | `true` to join the points |
 
 An option that doesn't apply to the chart's kind throws rather than being
 ignored, so a mistake is caught rather than silently dropped. Pie and doughnut
-charts colour their points individually rather than by series, which is why
+charts color their points individually rather than by series, which is why
 `color` has no meaning there.
 
-Without `color`, series take the theme's accent colours in turn, as Excel does.
+Without `color`, series take the theme's accent colors in turn, as Excel does.
 Anything not covered here is set afterwards with the functions in
 [Formatting charts](formattingCharts.md).
 
@@ -192,7 +192,7 @@ XLSX.Charts.Chart "chart1" on sheet "Data" at G9:N23
     [3] 2026 - Data!$D$2:$D$5 (4 pts)
 ```
 
-The new series takes the next accent colour and the chart's own style, so it
+The new series takes the next accent color and the chart's own style, so it
 looks like the ones already there.
 
 ```julia
@@ -208,10 +208,10 @@ to, so `addSeries` throws on one.
 
 ## Themes
 
-A created chart's colours are theme references, not fixed values, so the chart
+A created chart's colors are theme references, not fixed values, so the chart
 follows the workbook's theme the way Excel's own charts do.
 
 One consequence worth knowing: a workbook from [`XLSX.newxlsx()`](@ref) carries an older
 Office theme than current Excel, so charts created in a new workbook show that
-theme's colours rather than the ones Excel 365 would use. A chart added to a
+theme's colors rather than the ones Excel 365 would use. A chart added to a
 workbook Excel created uses that workbook's theme and matches.