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.

Sign up for GitHub

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

Jump to bottom

Replace many @aliases by @deprecate_binding #1469

Merged
merged 1 commit into from
Nov 23, 2023
Merged

Replace many @aliases by @deprecate_binding #1469

merged 1 commit into from
Nov 23, 2023

Conversation

fingolfin
Copy link
Member

This way any access to them trigger a depwarning (if those are enabled,
as they are in our CI).

This is a first step to eventually removing these aliases.

@fingolfin fingolfin force-pushed the mh/deprecate_binding branch from 863bbcc to 51f4b84 Compare October 17, 2023 15:47
@fingolfin fingolfin mentioned this pull request Oct 17, 2023
Merged
@fingolfin fingolfin force-pushed the mh/deprecate_binding branch from e269650 to d20bdcc Compare October 18, 2023 20:05
@codecov
Copy link

codecov bot commented Oct 18, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a825467) 87.11% compared to head (35f9870) 87.11%.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #1469   +/-   ##
=======================================
  Coverage   87.11%   87.11%           
=======================================
  Files         112      112           
  Lines       28786    28786           
=======================================
  Hits        25078    25078           
  Misses       3708     3708

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

@fingolfin fingolfin force-pushed the mh/deprecate_binding branch 2 times, most recently from 4999ee8 to 6fd9c32 Compare November 1, 2023 20:50
@fingolfin fingolfin mentioned this pull request Nov 2, 2023
Merged
@fingolfin fingolfin force-pushed the mh/deprecate_binding branch 3 times, most recently from 0fad5b4 to b751144 Compare November 7, 2023 10:13
@fingolfin
Copy link
Member Author

Discussed this yesterday with @fieker and others in our weekly KL OSCAR meeting, and we agreed that we should merge this... But paging @thofma to confirm he is OK with this? (Also I need to rebase this first now)

@fingolfin
Copy link
Member Author

(and note that we are doing a breaking release next)

@fingolfin fingolfin force-pushed the mh/deprecate_binding branch from b751144 to 627e8b6 Compare November 23, 2023 09:41
fingolfin
fingolfin commented Nov 23, 2023
View reviewed changes
src/Aliases.jl Outdated

@alias DirectSum direct_sum
Base.@deprecate_binding DirectSum direct_sum

# Deprecated in 0.34.*
@alias PuiseuxSeriesField puiseux_series_field
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not touch these "new" aliases because they were just introduced... but then again I think this is only used in Nemo, and we might just as well rip of the band aid now... @fieker @thofma thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(The comment with ripping off the band aid referred only to puiseux_series_field and puiseux_series_ring (resp. their old names), i.e.: we could use Base.@deprecate_binding for both right now...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think we should just do it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fieker also agrees, will do

fingolfin
fingolfin commented Nov 23, 2023
View reviewed changes
src/Generic.jl
Base.@deprecate_binding AbsSeriesRing AbsPowerSeriesRing
Base.@deprecate_binding AbsSeriesElem AbsPowerSeriesRingElem
Base.@deprecate_binding RelSeriesRing RelPowerSeriesRing
Base.@deprecate_binding RelSeriesElem RelPowerSeriesRingElem

# Deprecated in 0.34.*
@alias Frac FracFieldElem
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another new @alias I did not touch -- but Frac is used a lot in Oscar.jl, so maybe it's good to take this one slower, as there may be in-progress PRs in Oscar affected by this renaming...

@thofma
Copy link
Member

thofma commented Nov 23, 2023

Fine with me

@fingolfin fingolfin force-pushed the mh/deprecate_binding branch from 627e8b6 to 01f62d0 Compare November 23, 2023 10:23
@fingolfin
Replace many @Aliases by @deprecate_binding
35f9870 
This way any access to them trigger a depwarning (if those are enabled,
as they are in our CI).

This is a first step to eventually removing these aliases.
@fingolfin fingolfin force-pushed the mh/deprecate_binding branch from 01f62d0 to 35f9870 Compare November 23, 2023 10:33
@fingolfin fingolfin enabled auto-merge (squash) November 23, 2023 12:30
@fingolfin fingolfin disabled auto-merge November 23, 2023 12:31
@fingolfin fingolfin merged commit 9c397c8 into Nemocas:master Nov 23, 2023
15 checks passed
@fingolfin fingolfin deleted the mh/deprecate_binding branch November 23, 2023 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thofma thofma thofma left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@fingolfin @thofma