Update documents

[hyrodium]

• Update docstrings with jldoctest
• Fix stable docs link to LuxorManual
• Update language specifications in code blocks
• Update docs/make.jl format (indent etc.)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7fa91e1) 73.94% compared to head (6df6d2a) 73.94%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #295   +/-   ##
=======================================
  Coverage   73.94%   73.94%           
=======================================
  Files          36       36           
  Lines        6687     6687           
=======================================
  Hits         4945     4945           
  Misses       1742     1742           

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

[cormullion]

Thanks!

[cormullion]

Don't know what these 'doctests' are...

https://github.com/JuliaGraphics/Luxor.jl/actions/runs/7948031755/job/21697557018#step:6:12

[hyrodium]

Hmm, it should be fixed by this line..

https://github.com/JuliaGraphics/Luxor.jl/pull/295/files#diff-4aae2d1c783cade58bd2cb13748da956e568b7f2aed5fafd9e2a46fb97daf613R4

I'll take a look.

[hyrodium]

Ah, that was because of these lines in ci.yml:

Luxor.jl/.github/workflows/ci.yml

Lines 74 to 79 in 80a780b

	- run: |
	julia --project=docs -e '
	using Documenter: doctest
	using Luxor
	doctest(Luxor)'
	- run: julia --project=docs docs/make.jl

I'll open a PR with julia-docdeploy workflow.

[cormullion]

Cool, thanks. To be honest, I thought you were just going to add a deprecation for a Point method ready for a 3.9 release - there seem to be many more changes happening now 🤔

[hyrodium]

Sorry for not having enough explanation!

Implementation

I initially thought we could just deprecate and remove methods such as +(::Point, ::Number), but that was NOT true. Removing the method breaks broadcasting such as Point(1,2) .+ 3, and I needed to rewrite the entire broadcasting system (#294). That resulted breaking code like rule.(between.(O - (250, 0), O + (250, 0), 0:0.01:1), -π/3).

There are three possibilities for this situation.

• Remove the methods and update broadcasting.
  • This is what I've done in Remove methods like +(::Number, ::Point) and update around broadcasting #294.
  • This requires the Ref calls.
• Remove the methods, update broadcasting, and keep rule.(between.(O - (250, 0), O + (250, 0), 0:0.01:1), -π/3) working.
  • This is not consistent with Base behavior (e.g. we need Ref in norm.(Ref([1,2]), 2:3))
  • I'm not sure we can satisfy both Base.broadcastable(x::Point) = Ref(x) and removing methods like +(::Point, ::Number).
• Keep the methods +(::Point, ::Number) and broadcasting.
  • Of course, enforcing users to write Ref may be troublesome and could conflict with Luxor's simplicity.
  • Reverting Remove methods like +(::Number, ::Point) and update around broadcasting #294 would be one possibility!

My first question: Is it okay to keep the first one?

Deprecation

I initially thought we need just deprecations for methods like +(::Point, ::Number), but we now also need deprecations for broadcasting such as rule.(between.(O - (250, 0), O + (250, 0), 0:0.01:1), -π/3). Releasing v3.9.0 with the following properties would be hard or impossible (as discussed in the second bullet in the previous section).

• Keep Point(1,2) + 3 etc. working, and add deprecations to that.
• Keep Point(1,2) .+ 3 etc. working, and avoid warnings to that.
• Keep rule.(between.(O - (250, 0), O + (250, 0), 0:0.01:1), -π/3) working, and add deprecations to that.
• Keep rule.(between.(Ref(O - (250, 0)), Ref(O + (250, 0)), 0:0.01:1), -π/3) working, and avoid warnings to that.

My second question: Is it acceptable to avoid releasing v3.9.0? If not, what kind of deprecations should I add?

Documentation

Luxor.jl v4 breaks code like rule.(between.(O - (250, 0), O + (250, 0), 0:0.01:1), -π/3), so I'm trying updating all docs code with code blocks (@repl and @example) and generated PNG/SVG files. (i.e. we can remove image files such as https://github.com/JuliaGraphics/Luxor.jl/blob/master/docs/src/assets/figures/drawing_on_images.png from the default branch!)

I'm also working on JuliaDocs/Documenter.jl#2452, and I would like to keep Luxor.jl in the list.
Automatically generated images by Luxor in the docs are definitely good example, so woking with @repl and @example blocks is beneficial for this too.

My third question: Do we hurry for the next release? Updating the docs may take time a bit.

[cormullion]

Although I personally think the Ref syntax is unintuitive and ugly, but if it’s more correct to remove it than to keep it I won’t argue… 😀 People complain loudly about correctness so let’s not give them anything to complain about. (I may add a new function that does what I want 😂)

I don’t think the example page in the Documenter docs is worth worrying about. Luxor was a very early entry, but now there are thousands - including the official Julia docs. I think that the section could be removed.

I think we can release v4 without adding deprecations to an existing release ? That’s what I’d do. 🤔

You’ve taken on a lot of work - thank you - take your time!

[hyrodium]

Thank you for the reply!

People complain loudly about correctness so let’s not give them anything to complain about.

Exactly 😂😂

I think we can release v4 without adding deprecations to an existing release ? That’s what I’d do. 🤔

Agreed! Let's release v4.0.0 after updating the docs!

[hyrodium]

Although I personally think the Ref syntax is unintuitive and ugly, but if it’s more correct to remove it than to keep it I won’t argue…

I found another fourth possibility for the issue. Prohibiting Point(1,2) .+ 1 and accepting rule.(between.(O - (250, 0), O + (250, 0), 0:0.01:1), -π/3). This is a similar approach to String.

julia> getindex.("abc", 2:3)  # Do not need `Ref` to `"abc"`
2-element Vector{Char}:
 'b': ASCII/Unicode U+0062 (category Ll: Letter, lowercase)
 'c': ASCII/Unicode U+0063 (category Ll: Letter, lowercase)

julia> "abc" .+ 1  # Do not return `"bcd"`. Note that `['a', 'b', 'c'] .+ 1 == ['b', 'c', 'd']`
ERROR: MethodError: no method matching +(::String, ::Int64)

Closest candidates are:
  +(::Any, ::Any, ::Any, ::Any...)
   @ Base operators.jl:587
  +(::Base.CoreLogging.LogLevel, ::Integer)
   @ Base logging.jl:131
  +(::Complex{Bool}, ::Real)
   @ Base complex.jl:320
  ...

So, my current question is which one of these codes can be dropped?

• Drop rule.(between.(O - (250, 0), O + (250, 0), 0:0.01:1), -π/3)
  • One needs Ref to avoid errors.
  • The current approach.
• Drop Point(1,2) .+ 3
  • One can write explicitly call Point(1,2) + Point(3,3)
  • Point(1,2) .^ 3 will be more complicated, but that should not be a common use case.
  • The proposed approach in this comment

I really don't have much preference about this, but I think we need to reconsider the interface before releasing v4.

[cormullion]

I can understand that 'Point(1,2) .+ 3' (#270) is a valid concern, and it might be the cause of some misunderstandings, so perhaps it should not be available.

Also I think It's easy to add methods that avoid broadcasting. For example these are easy to add and easier to understand than the Ref versions:

function between(pt1::Point, pt2::Point, r::Real)

function between(pt1::Point, pt2::Point, r::AbstractRange)

function between(pt1::Point, pt2::Point, a::AbstractArray)

[hyrodium]

Sorry for the late response. I now think we should drop support for Point(1,2) .+ 3.
I will open a PR for that this weekend!