Make Test a weak dependency

[devmotion]

Fixes #51.

[aplavin]

See JuliaObjects/ConstructionBase.jl#86 for potential issues. Issues are likely temporary and will be fixed in newer Julia versions, but exist for now.

Also, not sure why such long extension names... Is there still anything out there that requires prefixes?

[devmotion]

Also, not sure why such long extension names... Is there still anything out there that requires prefixes?

It's the standard convention. I don't think there's a technical requirement but AFAIK it has been the recommendation from the start and what's typically used by most packages.

[devmotion]

Issues are likely temporary and will be fixed in newer Julia versions, but exist for now.

I guess you're referring to JuliaLang/julia#52841? I've only seen the issue in CI and AFAICT the fix was backported to julia 1.10.1. Since InverseFunctions already has a weak dependency on Dates and many other packages in the ecosystem (in SciML or eg Distributions) use the same approach for Test, this PR most likely is not the only problem if you actually run into this bug. To me it feels fine to re-consider the extensions (or guard them) only if users actually run into problems.

[nhz2]

Just FYI, I am currently in the process of removing the test extension in TranscodingStreams.jl: JuliaIO/TranscodingStreams.jl#235 due to strange errors users have encountered: JuliaIO/TranscodingStreams.jl#234 JuliaIO/TranscodingStreams.jl#223

An alternative to JuliaIO/TranscodingStreams.jl#235 was JuliaIO/TranscodingStreams.jl#236 where I removed the Test dependency by basically replacing @test x == y with x == y || error("test failed, add special message here")

[aplavin]

I've only seen the issue in CI and AFAICT the fix was backported to julia 1.10.1.

A (part of the original?) issue regarding extension loading is still present on 1.10 and 1.11rc: see JuliaLang/julia#52511, reproducer is right there at the bottom.
The fix may come in 1.11 and 1.10.5 (JuliaLang/julia#54658), or it may still be WIP (JuliaLang/julia#54750).

Since InverseFunctions already has a weak dependency on Dates and many other packages in the ecosystem (in SciML or eg Distributions) use the same approach for Test

I'm not sure when exactly the issue triggers, it may be related to some stdlibs or some specific loading order... Just be very careful making such moves (with known related Julia issues) in a package that's potentially deep into dependency trees.

[aplavin]

It's the standard convention. I don't think there's a technical requirement but AFAIK it has been the recommendation from the start

There was a very brief period during 1.9rc, when extensions were experimental and some tooling didn't work without these prefixes. This recommendation dates back to those times, it was just never updated...
Many people agree that it should be updated to recommend short names, see discussion at JuliaLang/Pkg.jl#3600. It's just that noone came to do it properly :)

and what's typically used by most packages

I see both PackageAnotherPackageExt.jl and AnotherPackageExt.jl in juliahub search, both are used (non-complete list of short named exts: https://juliahub.com/ui/Search?type=code&q=module+[A-Z][a-z]%2BExt&rx=true&w=true).
The former does seem somewhat more popular, but I think it's mostly packages that added long-named extensions at some point and keep them to avoid unnecessary changes. Meanwhile, InverseFuctions, same as many other packages, has been using short ext naming for quite some time.

[ChrisRackauckas]

It is non-breaking to change an extension name in the future, so I don't see a reason for that discussion to block this PR. Please feel free to rename in a future PR if you see it necessary.

[oschulz]

We should do the same for ChangesOfVariables , I guess.

[aplavin]

Tbh, the review/release process I saw here makes me very concerned about the priorities of InverseFunctions devs.
This PR was merged and force-registered in a few hours, without directly addressing the relevant Julia issue that we know is still present. That issue may or may not be manifested in this case specifically, but how can we know beforehand (if you know, just explain that)? And why not wait for its full fix that may come in the next julia release?

This PR doesn't add new features nor fixes any bugs, so not sure what was the hurry. Moreover, this PR is breaking (even though unlikely to break anything in practice). Meanwhile, a bugfix PR to InverseFunctions had been hanging for more than a year already.

Removing unnecessary dependencies is nice, but doing that without regards to known issues, and putting more priority to this than bugfixes?.. That I really didn't expect from such a package that can be deeply depended upon by many others.

It is non-breaking to change an extension name in the future, so I don't see a reason for that discussion to block this PR. Please feel free to rename in a future PR if you see it necessary.

This is definitely not the main concern here, it's weird that you only addressed this minor comment.
It just appears strange that this PR includes renaming of existing modules, completely unrelated to the actual change. Imagine if every commit did random renames throughout the package :)

[oschulz]

We should do the same for ChangesOfVariables , I guess.

@aplavin you'd recommend I hold off with that for now, then?

I'll let @devmotion speak to the merge/release here.

I'm not familiar with this issue, so I was wondering, in what package combinations does this problem occur? I just ask because so many packages use extensions now and I haven't run into trouble with extensions for a long time myself (there was certainly trouble early in v1.9, esp. with Plots and IJulia, of course).

[aplavin]

@aplavin you'd recommend I hold off with that for now, then?

In Accessors, we first moved stuff to extensions, and users reported this issue. There, we are kinda waiting for it to be fully fixed in Julia to reintroduce weakdeps.
It's not an urgent thing in any way, shaving a few stdlib deps is just a small nice-to-have. So, not seeing any harm in delaying it a bit.

For me personally, bugfixes and even new features are definitely of a higher priority than that.

I'm not familiar with this issue, so I was wondering, in what package combinations does this problem occur?

I'm not fully certain, but seems related to (some?) stdlibs. I never saw it with non-stdlib deps, and stdlibs also often work fine... But not always.

[oschulz]

Is this (Test extension in InverseFunctions) causing and issues for Accessors & friends, or can we leave things the way they are now?

[aplavin]

I'm not going to have opportunities to check that for the next few days unfortunately.

[oschulz]

I just ran the Accessors and AccessorsExtra tests (Julia v1.10), all fine.

[devmotion]

IMO we should keep the changes for now and only revert (or fix it in some other way) if there are actual problems with it.

Based on the discussion in JuliaLang/julia#52511, it seems (JuliaLang/julia#52511 (comment)) that one of the problems just occurs when there are >= 2 extensions + a transitive dependency triggers them. So it seems (JuliaLang/julia#52132 (comment)) making Test a weak dependency of InverseFunctions might actually fix the original issue in Accessors, and generally this problem should not show up if all packages make Test a weak dependency.

[aplavin]

I just ran the Accessors and AccessorsExtra tests (Julia v1.10), all fine.

We never saw this issue manifested in Accessors tests, even when it had those weakdeps.

The two reliable reproducers I know of are:

]add AccessorsExtra@0.1.71
]add Accessors@0.1.34 SatelliteToolbox@0.12.2

(in an empty depot, latest 1.10.4 release)

Just checked, they don't seem to show any new warnings related to InverseFunctions. Either the issue isn't triggered here, or another example is needed – these two are for triggering Accessors-related warnings.