Skip to content

Add compatibility for view#238

Merged
TotalVerb merged 6 commits intoJuliaLang:masterfrom
ranjanan:RA/add-view
Jun 28, 2016
Merged

Add compatibility for view#238
TotalVerb merged 6 commits intoJuliaLang:masterfrom
ranjanan:RA/add-view

Conversation

@ranjanan
Copy link
Copy Markdown
Contributor

@ranjanan ranjanan commented Jun 27, 2016

Test added, README modified. Fixes #235 .

@ranjanan ranjanan mentioned this pull request Jun 27, 2016
src/Compat.jl Outdated
end

if isdefined(Base, :view)
nothing
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You should reverse the branch condition and delete this....

test/runtests.jl Outdated

# Add test for Base.view
let a = rand(10,10)
@test view(a, 1, :) == slice(a, 1, :)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Won't this trigger a depwarn on 0.5? Maybe construct a view and compare it with getindex and check that mutating the view aliases the original one.

test/runtests.jl Outdated

# Add test for Base.view
let a = rand(10,10)
@test Array(view(a, :, 1)) == a[:,1]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The Array shouldn't be necessary.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You are right, it seems to work without that.

README.md Outdated

* `write(::IO, ::Ptr, len)` is now `unsafe_write` [#14766](https://github.com/JuliaLang/julia/pull/14766).

* `slice` is now `view`: do `import Compat.view` and then use `view` normally without the `@compat` macro.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

reference the julia PR - I think it was by @simonbyrne if that helps find it faster

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this: JuliaLang/julia#16972

@yuyichao
Copy link
Copy Markdown
Contributor

LGTM (need squash or squash on merge)

@ranjanan
Copy link
Copy Markdown
Contributor Author

@tkelman or someone, can we merge, and tag a new release please?

@TotalVerb TotalVerb merged commit 0a37517 into JuliaLang:master Jun 28, 2016
@ranjanan
Copy link
Copy Markdown
Contributor Author

@TotalVerb can you tag a new release please? There are packages that are dependent on this. Thanks!

@TotalVerb
Copy link
Copy Markdown
Contributor

TotalVerb commented Jun 28, 2016

I will once CI passes. Thanks for the contribution!

@TotalVerb
Copy link
Copy Markdown
Contributor

@tkelman I can't figure out how to tag a release. Could you handle this one?

@timholy
Copy link
Copy Markdown
Member

timholy commented Jun 28, 2016

dpsanders pushed a commit to dpsanders/Compat.jl that referenced this pull request Feb 1, 2017
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.

5 participants