Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix uses of Val inside Hecke #1455

Merged
merged 31 commits into from
Apr 18, 2024
Merged

Fix uses of Val inside Hecke #1455

merged 31 commits into from
Apr 18, 2024

Conversation

lgoettgens
Copy link
Contributor

This is similar to Nemocas/AbstractAlgebra.jl#1664.

Val{T} is a type and Val(T) its singleton object. This is the way it is described in the julia docs (https://docs.julialang.org/en/v1/manual/types/#%22Value-types%22).

I went through all the uses and corrected them.

For all exported functions where the parameter occurred in a docstring, I already added deprecations. If you want more deprecations, please let me know.

src/AlgAss/AlgGrp.jl Outdated Show resolved Hide resolved
@lgoettgens
Copy link
Contributor Author

I'll continue to add deprecations for everything that fails in the downstream tests

@lgoettgens
Copy link
Contributor Author

Somehow the "fake" doctests in Oscar emit deprecation warnings that make them fail. Do you have an idea why depwarns are enabled there?

@thofma
Copy link
Owner

thofma commented Apr 17, 2024

Hm, I don't remember how those fake doctests are run. @benlorenz do you remember from the top of your head?

@benlorenz
Copy link
Contributor

These are run in a dummy-module with a dummy function:
https://github.com/oscar-system/Oscar.jl/blob/183cbe6a5e9c3dbdb7e851c00d083dbf3a059343/experimental/OrthogonalDiscriminants/test/data.jl#L79-L149

depwarn=yes is the julia-runtest default, we set it to depwarn=error for the extra tests.

@thofma
Copy link
Owner

thofma commented Apr 18, 2024

Maybe add the old method back in @lgoettgens?

@benlorenz
Copy link
Contributor

Deprecating exported functionality is considered breaking and should be done with a minor version bump instead of a patch-level bump.

@lgoettgens
Copy link
Contributor Author

lgoettgens commented Apr 18, 2024

Deprecating exported functionality is considered breaking and should be done with a minor version bump instead of a patch-level bump.

This is just stated for versions after 1.0.0 (ref https://semver.org/spec/v2.0.0.html#spec-item-7), in the 0.y.z phase everything may change at any time (https://semver.org/spec/v2.0.0.html#spec-item-4). (And in this case here force_coerce_cyclo isn't even exported.)

To reduce friction here, let me add the deprecations as "real functions" (i.e. without a @deprecate) and we change this in the next minor release instead.

@thofma
Copy link
Owner

thofma commented Apr 18, 2024

Julia does not follow SemVer for 0.*: https://pkgdocs.julialang.org/v1/compatibility/#compat-pre-1.0.

@benlorenz
Copy link
Contributor

(Almost) everything that is exported by Hecke is also exported by Oscar and thus breaking for Oscar 1.0.x.

@lgoettgens
Copy link
Contributor Author

(Almost) everything that is exported by Hecke is also exported by Oscar and thus breaking for Oscar 1.0.x.

Thanks for reminding me. However, this is no longer an issue due to my last changes. The deprecations will only be introduced in Hecke 0.31.0, which will then only be made compatible on the Oscar master branch, and not the release-1.0.

@benlorenz
Copy link
Contributor

Thanks!

In this case it seems it is technically a bug in Oscar since it relies on the non-exported Hecke.force_coerce_cyclo.

But with all the re-exports we should be very careful with such things and moving to a new minor release soon seems like a good idea. We can still create a branch for 0.30 and do bugfixes there if necessary.

@thofma thofma merged commit 32aba11 into thofma:master Apr 18, 2024
15 checks passed
@lgoettgens lgoettgens deleted the lg/Val-Hecke branch April 24, 2024 14:36
lgoettgens added a commit to lgoettgens/Hecke.jl that referenced this pull request Apr 26, 2024
thofma pushed a commit that referenced this pull request May 1, 2024
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.

3 participants