-
Notifications
You must be signed in to change notification settings - Fork 63
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
Enforce @attr
uses to always provide a return type
#1782
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1782 +/- ##
==========================================
- Coverage 88.03% 88.03% -0.01%
==========================================
Files 117 117
Lines 30094 30088 -6
==========================================
- Hits 26494 26488 -6
Misses 3600 3600 ☔ View full report in Codecov by Sentry. |
fingolfin
reviewed
Sep 4, 2024
fingolfin
approved these changes
Sep 4, 2024
@fingolfin the Singular test failure is unrelated. Can you force-merge this? |
Sorry, I am too slow, but I think this is a good idea. |
lgoettgens
added a commit
to lgoettgens/Hecke.jl
that referenced
this pull request
Sep 19, 2024
lgoettgens
added a commit
to lgoettgens/Hecke.jl
that referenced
this pull request
Sep 19, 2024
lgoettgens
added a commit
to lgoettgens/Hecke.jl
that referenced
this pull request
Sep 19, 2024
lgoettgens
added a commit
to lgoettgens/Hecke.jl
that referenced
this pull request
Sep 19, 2024
lgoettgens
added a commit
to lgoettgens/Hecke.jl
that referenced
this pull request
Sep 19, 2024
thofma
pushed a commit
to thofma/Hecke.jl
that referenced
this pull request
Sep 19, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We have a lot of places in our codebases where people use
@attr
without specifying a return type, even though in many of these cases they are very easy too deduce (see e.g. all of thedim
andis_*
cases in oscar-system/Oscar.jl#4059). I don't know if this is due to people not knowing about the possibility, forget it, or just don't care. But at least for me, it is usually the second one (see e.g. the Lie algebra changes in oscar-system/Oscar.jl#4059).Thus I propose to deprecate (in the next breaking release) the use of 1-arg
@attr
(with only a method expression) in favor of explicitly requiring people to write@attr Any
if they don't want to think of the return type. I think this would be a good measure for enforcing better inferrable code (by just making it harder to shadow inference by accident). And this is something that can be easily adapted by a search-and-replace when doing the AA compat bump downstream.As always, the change in
deprecations.jl
does not deprecate anything right now, it just providesAny
as the return type to the 2-arg version, and makes it easy to deprecate it in a future breaking version bump.