hyrodium commented 4 months ago

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

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

cormullion commented 4 months ago

Thanks!

cormullion commented 4 months ago

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

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

hyrodium commented 4 months ago

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 commented 4 months ago

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

https://github.com/JuliaGraphics/Luxor.jl/blob/80a780bcb00c9c1cb227a71b38acec2f13a561ff/.github/workflows/ci.yml#L74-L79

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

cormullion commented 4 months ago

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 commented 4 months ago

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 #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 #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 https://github.com/JuliaDocs/Documenter.jl/pull/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 commented 4 months ago

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 commented 4 months ago

Thank you for the reply!

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

Exactly :joy::joy:

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 commented 4 months ago

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 commented 4 months ago

I can understand that 'Point(1,2) .+ 3' (https://github.com/JuliaGraphics/Luxor.jl/issues/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 commented 4 months ago

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!

JuliaGraphics / Luxor.jl

Update documents #295

Codecov Report

Implementation

Deprecation

Documentation