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

Rename mul to * #2913

Merged
merged 4 commits into from
Oct 26, 2023
Merged

Rename mul to * #2913

merged 4 commits into from
Oct 26, 2023

Conversation

lgoettgens
Copy link
Member

Oscar side of thofma/Hecke.jl#1243

@benlorenz
Copy link
Member

The corresponding Hecke version was released and the deprecation broke the Oscar CI.
I merged master to fix the conflicts and let CI run, so we can hopefully fix the CI soon.

@lgoettgens
Copy link
Member Author

Thanks for working on this! I restarted all ci jobs that failed (mostly due to overly long build steps). But overall, it seems to be fine after your change.

@thofma
Copy link
Collaborator

thofma commented Oct 26, 2023

@benlorenz is there something we need to change in the downstream tests to catch this? I am pretty sure they showed success.

@thofma thofma enabled auto-merge (squash) October 26, 2023 05:43
@lgoettgens
Copy link
Member Author

@benlorenz is there something we need to change in the downstream tests to catch this? I am pretty sure they showed success.

This failure is expected with the current setup in Oscar. I created #2956 as a try to change that for the future.

@thofma
Copy link
Collaborator

thofma commented Oct 26, 2023

It is fine that Oscar expects this. It is not fine, that this is not catched in downstream tests. How does #2956 fix this? Shouldn't the fix involve https://github.com/oscar-system/OscarDevTools.jl?

@benlorenz
Copy link
Member

It is fine that Oscar expects this. It is not fine, that this is not catched in downstream tests. How does #2956 fix this?

It doesn't strictly fix this it just avoids breaking the whole CI and instead shows just one failing job when Oscar triggers deprecations.
Using deprecated functions should not block us from merging other PRs, especially since the deprecations might not even come from code in the Oscar ecosystem (last time we discussed this it came from JLLWrappers).

Shouldn't the fix involve oscar-system/OscarDevTools.jl?

We could enable depwarn=error there as well but I am not sure if this is the best approach, as explained above.
Fixing deprecations is not really something that needs to be done immediately.

@codecov
Copy link

codecov bot commented Oct 26, 2023

Codecov Report

Merging #2913 (1502297) into master (b6c8dc8) will decrease coverage by 0.06%.
The diff coverage is 80.76%.

@@            Coverage Diff             @@
##           master    #2913      +/-   ##
==========================================
- Coverage   80.41%   80.35%   -0.06%     
==========================================
  Files         470      470              
  Lines       66601    66648      +47     
==========================================
- Hits        53556    53555       -1     
- Misses      13045    13093      +48     
Files Coverage Δ
src/Groups/GAPGroups.jl 92.79% <ø> (-0.02%) ⬇️
src/Modules/Posur.jl 73.77% <100.00%> (ø)
src/Modules/UngradedModules.jl 83.36% <100.00%> (ø)
src/Modules/mpolyquo-localizations.jl 88.88% <100.00%> (ø)
src/deprecations.jl 0.00% <ø> (ø)
src/Modules/mpoly-localizations.jl 70.64% <0.00%> (ø)
src/Rings/mpoly-localizations.jl 77.08% <73.33%> (ø)

... and 6 files with indirect coverage changes

@thofma thofma merged commit acd5903 into oscar-system:master Oct 26, 2023
14 of 19 checks passed
@lgoettgens lgoettgens deleted the lg/mul branch October 26, 2023 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants