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

Compat for Julia #18510, #18484 #287

Merged
merged 2 commits into from Oct 18, 2016
Merged

Compat for Julia #18510, #18484 #287

merged 2 commits into from Oct 18, 2016

Conversation

davidagold
Copy link
Contributor

@tkelman
Copy link
Contributor

tkelman commented Oct 17, 2016

thanks, add to README

@davidagold davidagold force-pushed the dag/nullable branch 4 times, most recently from 709be58 to 50656ea Compare October 17, 2016 17:06
tkelman
tkelman approved these changes Oct 17, 2016
View reviewed changes
Copy link
Contributor

@tkelman tkelman left a comment

Choose a reason for hiding this comment

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

lgtm!

@davidagold
Copy link
Contributor Author

Thanks! cc @nalimilan who may be interested

@davidagold
Copy link
Contributor Author

@tkelman how many reviews does this need?

TotalVerb
TotalVerb suggested changes Oct 18, 2016
View reviewed changes
src/Compat.jl
@@ -613,6 +613,8 @@ function _compat(ex::Expr)
elseif VERSION < v"0.5.0-dev+4340" && length(ex.args) > 3 &&
istopsymbol(withincurly(ex.args[1]), :Base, :show)
ex = rewrite_show(ex)
elseif VERSION < v"0.6.0-dev.826" && f === :Nullable && length(ex.args) == 3 # julia#18510
ex = Expr(:call, :Nullable, ex.args[2], Expr(:call, :(Compat._Nullable_field2), ex.args[3]))
Copy link
Contributor

@TotalVerb TotalVerb Oct 18, 2016 •
edited

Choose a reason for hiding this comment

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

This does not handle Nullable{Int}(x, b) properly. See how show is handled above (need withincurly and istopsymbol).

i.e. condition should be istopsymbol(withincurly(f), :Base, :Nullable) and return value should be Expr(:call, f, ex.args[2], ...)

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM except for this.

@davidagold
Copy link
Contributor Author

Good catch -- thanks, @TotalVerb . I've made those changes and added tests.

TotalVerb
TotalVerb suggested changes Oct 18, 2016
View reviewed changes
src/Compat.jl
@@ -613,6 +613,9 @@ function _compat(ex::Expr)
elseif VERSION < v"0.5.0-dev+4340" && length(ex.args) > 3 &&
istopsymbol(withincurly(ex.args[1]), :Base, :show)
ex = rewrite_show(ex)
elseif VERSION < v"0.6.0-dev.826" && length(ex.args) == 3 && # julia#18510
istopsymbol(withincurly(ex.args[1]), :Base, :Nullable)
ex = Expr(:call, :Nullable, ex.args[2], Expr(:call, :(Compat._Nullable_field2), ex.args[3]))
Copy link
Contributor

@TotalVerb TotalVerb Oct 18, 2016 •
edited

Choose a reason for hiding this comment

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

Expr(:call, f or Expr(:call, ex.args[1]. Currently it doesn't behave properly for Nullable{Any}(1.0, false) (maybe add that as a test).

@davidagold
Copy link
Contributor Author

Thanks again for your thoroughness

TotalVerb
TotalVerb approved these changes Oct 18, 2016
View reviewed changes
Copy link
Contributor

@TotalVerb TotalVerb left a comment

Choose a reason for hiding this comment

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

LGTM.

@TotalVerb
Make Nullable{Any} tests a tad bit more strict
ed595af 
Previous tests would pass even if `Nullable{Any}` was rewritten to `Nullable`.
@TotalVerb
Copy link
Contributor

@davidagold I added two test cases for eltype of Nullable{Any}. I think this is good to merge now.

@TotalVerb TotalVerb merged commit 2534435 into JuliaLang:master Oct 18, 2016
@davidagold davidagold deleted the dag/nullable branch October 18, 2016 16:33
@tkelman tkelman mentioned this pull request Oct 18, 2016
Merged
@tkelman
Copy link
Contributor

tkelman commented Oct 18, 2016

Assuming no-one objects to tagging this. JuliaLang/METADATA.jl#6791

martinholters added a commit that referenced this pull request Aug 27, 2018
@martinholters
Remove at-compat for Nullable construction
3479138 
Was added in #287, now obsolete as no longer required on minimum 
supported Julia version 0.6.
martinholters added a commit that referenced this pull request Aug 31, 2018
@martinholters
Weed out relicts from at-compat (#626)
96e41ae 
* Remove at-compat for type aliases

  Was added in #326, now obsolete as no longer required on minimum
  supported Julia version 0.6.

* Remove at-compat for Nullable construction

  Was added in #287, now obsolete as no longer required on minimum 
  supported Julia version 0.6.

* Remove at-compat for Foo{<:Bar} sugar

  Was added in #317 (and #336), now obsolete as no longer required on 
  minimum supported Julia version 0.6.

* Remove at-compat for index styles

  Was added in #329, now obsolete as no longer required on minimum 
  supported Julia version 0.6.

* Remove at-compat for type declarations

  Was added in #325, now obsolete as no longer required on minimum 
  supported Julia version 0.6.

* Remove unused at-compat helper functions

* Remove README entries for removed at-compat functionality

  * new style call overloading (added in #181, removed in #385)
  * `get(io, s false)` (added in #212, #215, #225, removed in #385)
  * `.=` (added in #292 and #316, removed in #372)

* Remove `Compat.collect(A)`

  Was added in #350 and #351, now obsolete as no longer required on 
  minimum supported Julia version 0.6.
martinholters added a commit that referenced this pull request Aug 31, 2018
@martinholters
Remove definitions of unsafe_get and isnull fallsback
f775fe7 
Were added in #287.
martinholters added a commit that referenced this pull request Aug 31, 2018
@martinholters
Remove definitions of unsafe_get and isnull fallsback
47db995 
Were added in #287.
stevengj pushed a commit that referenced this pull request Sep 5, 2018
@martinholters @stevengj
Remove a bunch of definitions only relevant prior to Julia 0.6 (#628)
ef9891d 
* Remove `take!(::Task)` definition for Julia versions prior to 0.6

Was added in #307.

* Remove `redirect_std*(f, stream)` definitions for Julia prior to v0.6

Were added in #275.

* Remove at-__DIR__ macro definition for Julia versions prior to 0.6

Was added in #281.

* Remove `broadcast` definition for equal-length tuples

Was added in #324 and #328

* Remove definitions of `unsafe_get` and `isnull` fallsback

Were added in #287.

* Remove defintions of `xor` and `⊻`

Were added in #289.

* Definitions of `numerator` and `denominator`

Were added in #290.

* Remove defintion of `iszero`

Was added in #305.

* Remove definition of `>:`

Was added in #336

* Remove definition of `take!(::Base.AbstractIOBuffer)`

Was added in #290.

* Remove definiton of `.&` and `.|`

Were added in #306.

* Remove definition of `Compat.isapprox`

Was added in #309.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tkelman tkelman tkelman approved these changes

@TotalVerb TotalVerb TotalVerb 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.

None yet
3 participants
@davidagold @tkelman @TotalVerb