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

#34518 - rename isimmutable to ismutable #34652

Merged
merged 9 commits into from
Feb 7, 2020
Merged

#34518 - rename isimmutable to ismutable #34652

merged 9 commits into from
Feb 7, 2020

Conversation

ssikdar1
Copy link
Contributor

@ssikdar1 ssikdar1 commented Feb 4, 2020 •
edited
Loading

PR for issue #34518.
Created a ismutable in base/reflection.jl and added a depwar to isimmutable.

Result in REPL:

julia> isimmutable(1)
┌ Warning:  isimmutable is deprecated use ismutable instead 
│   caller = top-level scope at REPL[1]:1
└ @ Core REPL[1]:1
true

julia> ismutable(1)
false

Switched isimmutable to ismutable in other parts of the code.

  • add isimmutable
  • replace isimmutable to ismutable
  • Add a few tests.
    Didn't add any new tests but test/reflection.jl passed.
$ ./julia test/reflection.jl
Test Summary: |
issue #31687  | No tests
Test Summary:       | Pass  Total
methods with module |   11     11
  • Add a NEWS item.

timholy
timholy reviewed Feb 4, 2020
View reviewed changes
base/reflection.jl Outdated
@@ -417,7 +417,28 @@ julia> isimmutable([1,2])
false
```
"""
isimmutable(@nospecialize(x)) = (@_pure_meta; !typeof(x).mutable)
function isimmutable(@nospecialize(x))
Copy link
Member

Choose a reason for hiding this comment

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

We generally don't start introducing "live" depwarns of exported functions in the middle of a stable release. Instead you should move this to the end of base/deprecated.jl but don't actually issue the depwarn. Most or all of the "live" depwarns there are for types or methods that were never exported in the first place and therefore not protected by the "no breaking changes" rule. Thus, those deprecations are not necessary but are there as a kindness. In contrast, this is part of Julia's public API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok! moved isimmutable from reflections.jl to the bottom of deprecated.jl in 6b76c59

timholy
timholy reviewed Feb 4, 2020
View reviewed changes
Copy link
Member

@timholy timholy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looking good. One more ask and I would approve this for merging. (Oh, but please do add the NEWS and a test of ismutable directly.)

base/deprecated.jl

Return `true` iff value `v` is immutable. See [Mutable Composite Types](@ref)
for a discussion of immutability. Note that this function works on values, so if you give it
a type, it will tell you that a value of `DataType` is mutable.
Copy link
Member

Choose a reason for hiding this comment

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

Sorry I didn't mention this before, but perhaps it would be good to add

!!! warning
    Consider using `!ismutable(v)` instead, as `isimmutable(v)` will be replaced by `!ismutable(v)` in a future release.

Then the last step for a truly awesome contribution would be to add ismutable to https://github.com/JuliaLang/Compat.jl.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Warning message added in a77d03a

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Compat PR JuliaLang/Compat.jl#686

@timholy timholy added the needs news A NEWS entry is required for this change label Feb 4, 2020
@ssikdar1
Copy link
Contributor Author

ssikdar1 commented Feb 4, 2020 •
edited
Loading

@timholy sure!

please do add the NEWS and a test of ismutable directly.

In the PR , I changed the tests in test/reflection.jl to use ismutable instead of isimmuatable
https://github.com/JuliaLang/julia/pull/34652/files#diff-2ea596942a7d251702840ffcaab1e763R107-R108

Is there somewhere else I should test ismutable ? Should I revert this?

@ssikdar1 ssikdar1 mentioned this pull request Feb 5, 2020
Merged
fredrikekre
fredrikekre reviewed Feb 5, 2020
View reviewed changes
base/deprecated.jl Outdated
"""
isimmutable(v) -> Bool
!!! warning
Consider using `!ismutable(v)` instead, as `isimmutable(v)` will be replaced by `!ismutable(v)` in a future release.
Copy link
Member

Choose a reason for hiding this comment

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

Would be good if this mentioned that ismutable was introduced in Julia v1.5.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added in (Since Julia 1.5) in a252f5e

base/deprecated.jl Outdated
false
```
"""
isimmutable(@nospecialize(x)) = (@_pure_meta; !typeof(x).mutable)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this could now be implemented as

Suggested change
isimmutable(@nospecialize(x)) = (@_pure_meta; !typeof(x).mutable)
isimmutable(@nospecialize(x)) = !ismutable(x)

but perhaps not that important.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed in eed8b67

base/exports.jl Outdated
@@ -642,6 +642,7 @@ export
isbits,
isequal,
isimmutable,
Copy link
Member

Choose a reason for hiding this comment

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

You could move this export to where the function is defined in deprecated.jl such that we don't forget to remove it once the function definition is removed (that has happened before).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

export moved to deprecated.jl in 28f7f9f

doc/src/base/base.md
@@ -159,6 +159,7 @@ Base.isdispatchtuple

```@docs
Base.isimmutable
Base.ismutable
Copy link
Member

Choose a reason for hiding this comment

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

Maybe switch the order and put ismutable first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

order switched in aba863f

@JeffBezanson JeffBezanson merged commit 1297b51 into JuliaLang:master Feb 7, 2020
@timholy
Copy link
Member

timholy commented Feb 7, 2020

Thanks @ssikdar1!

@fredrikekre fredrikekre removed the needs news A NEWS entry is required for this change label Feb 8, 2020
KristofferC pushed a commit that referenced this pull request Apr 11, 2020
@ssikdar1 @KristofferC
#34518 - replace isimmutable with ismutable (#34652)
4f02bc3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fredrikekre fredrikekre fredrikekre left review comments

@timholy timholy timholy approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@ssikdar1 @timholy @fredrikekre @JeffBezanson