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.

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

Add Compat.Unicode for standard library Unicode #432

Merged
merged 4 commits into from
Dec 21, 2017
Merged

Conversation

omus
Copy link
Member

@omus omus commented Dec 15, 2017

Changed in JuliaLang/julia#25021. At the moment I disabled the OffsetArray tests as they cause the tests to fail on Julia 0.7.

textwidth(c::AbstractString) = strwidth(c)
export textwidth
end

Copy link
Member Author

@omus omus Dec 15, 2017

Choose a reason for hiding this comment

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

I'm not sure what the procedure with deprecating a Compat supported change is. Since the rule for Compat.jl is to use the latest syntax it seemed reasonable to move textwidth into the Unicode module. Do I need to deprecate this method so that it's not breaking?

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer a deprecation over a breaking change. It is quite common for packages to require a fairly recent Compat version, so if others need to set an upper bound, that could be rather unfortunate.

But I have no idea want the standard procedure is.

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 added a deprecation for this so the transition should be non-breaking.

test/runtests.jl Outdated
# a = OffsetArray(1:3, -1:1)
# b = Compat.collect(a)
# @test indices(b) === (Base.OneTo(3),)
# @test b == [1,2,3]
Copy link
Member Author

Choose a reason for hiding this comment

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

Generates the error:

ERROR: LoadError: size not supported for arrays with indices (-1:1,); see http://docs.julialang.org/en/latest/devdocs/offset-arrays/

I haven't looked into this.

module. Code can be written to work on both 0.5 and 0.6 by `import`ing or
`using` the `Compat.Iterators` module instead. ([#18839])
* `using Compat.Unicode` is provided on versions older than 0.7, where this library is not
yet a part of the standard library. ([#25021])
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 diff is messed up here. I moved all of the using X notes into the "Module Aliases" section.

src/Compat.jl Outdated
@@ -874,6 +867,43 @@ end
export ComplexF64
end

# 0.7.0-DEV.2915
module Unicode
export normalize, graphemes, isassigned, textwidth, isvalid,
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 should remove normalize and isassigned here. JuliaLang/julia#25079

src/Compat.jl Outdated
end
end
return String(take!(b))
end
Copy link
Member Author

Choose a reason for hiding this comment

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

Failure on 0.6 due to titlecase being exported by Base in 6f8e94e3ff (JuliaLang/julia#19469)

@omus
Copy link
Member Author

omus commented Dec 21, 2017

Needs #435 and OffsetArrays to pass on 0.7

@omus omus changed the title WIP: Add Compat.Unicode for standard library Unicode Add Compat.Unicode for standard library Unicode Dec 21, 2017
@ararslan ararslan merged commit 7695d5c into master Dec 21, 2017
@ararslan ararslan deleted the cv/unicode branch December 21, 2017 04:08
@@ -188,3 +188,7 @@ else
import Base.@irrational
import Base.LinAlg.BLAS.@blasfunc
end

if VERSION < v"0.7.0-DEV.2915"
Base.@deprecate textwidth Compat.Unicode.textwidth
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the justification for these lines. They are annoying as they print deprecation warnings on Julia 0.6. These can appear for packages which worked perfectly well before, just because Compat has been updated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since textwidth needed to be moved inside Compat.Unicode module the only two real choices were to either make a alias at the top-level or to add a deprecation. I ended up going for the deprecation since Compat is about supporting "the latest syntax in a backwards-compatible way".

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 personally haven't seen this scenario where previous Compat code was required to be moved. There may be a standard procedure on how to handle this for which I am unaware.

Copy link
Member

Choose a reason for hiding this comment

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

OK, I see the logic. However I think it's bad to generate tons of deprecation warnings on Julia 0.6 for packages which worked perfectly fine until now. For example, DataFrames is very painful to use right now because of this.

I think the best approach is to keep exporting this function on Julia < 0.7 since it's supported by Julia 0.6, but not on Julia 0.7. We don't need to deprecate the function ourselves since Julia takes care of that already. See #437.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good to me. We should think about making a CONTRIBUTING.md to Compat.jl to clarify these kind of details.

amellnik added a commit to amellnik/LibExpat.jl that referenced this pull request Jan 2, 2018
Importing via Compat (see JuliaLang/Compat.jl#432) to keep 0.6 compatability.
martinholters added a commit that referenced this pull request Oct 4, 2019
martinholters added a commit that referenced this pull request Oct 4, 2019
martinholters added a commit that referenced this pull request Oct 8, 2019
martinholters added a commit that referenced this pull request Oct 13, 2019
* Bump required Julia version to 1.0

* Remove compatibility support code for:
  * `at-__MODULE__` (from #363)
  * `devnull`, `stdin`, `stdout`, and `stderr` from #499
  * `at-nospecialize` (from #385 and #409)
  * `isabstracttype` and `isconcretetype` (from #477)
  * `invokelatest` from #424
  * array-like access to `Cmd` from #379
  * `Val(n)` and `ntuple`/`reshape` with `Val` from #381 and #399
  * `logdet(::Any)` fallback from #382
  * `chol(::UniformScaling)` from #382
  * `pushfirst!`, `popfirst!` from #444
  * `fieldcount` from #386
  * `read(obj, ::Type{String})` from #385 and #580
  * `InexactError`, `DomainError`, and `OverflowError` constructors from #393
  * `corrected` kw arg to `cov` from #401
  * `adjoint` from #401
  * `partialsort` from #401
  * `pairs` from #428
  * `AbstractRange` from #400
  * `rtoldefault` from #401
  * `Dates.Period` rounding from #462
  * `IterativeEigensolvers` from #435
  * `occursin` from #520
  * `Char` concatenation from #406
  * `BitSet` from #407
  * `diagm` and `spdiagm` with pairs from #408
  * `Array` c'tors from `UniformScaling` from #412 and #438
  * `IOContext` ctor taking pairs from #427
  * `undef` from #417 and #514
  * `get` on `ENV` from #430
  * `ComplexF...` from #431
  * `tr` from #614
  * `textwidth` from #644
  * `isnumeric` from #543
  * `AbstractDict` from #435
  * `axes` #435 and #442
  * `Nothing` and `Cvoid` from #435
  * `Compat.SuiteSparse` from #435
  * `invpermute!` from #445
  * `replace` with a pair from #445
  * `copyto!` from #448
  * `contains` from #452
  * `CartesianIndices` and `LinearIndices` from #446, #455, and #524
  * `findall` from #466 (and #467).
  * `argmin` and `argmax` from #470, #472, and #622
  * `parentmodule` from #461
  * `codeunits` from #474
  * `nameof` from #471
  * `GC` from #477
  * `AbstractDisplay` from #482
  * `bytesavailable` from #483
  * `firstindex` and `lastindex` from #480 and #494
  * `printstyled` from #481
  * `hasmethod` from #486
  * `objectid` from #486
  * `Compat.find*` from #484 and #513
  * `repr` and `showable` from #497
  * `Compat.names` from #493 and #505
  * `Compat.round` and friends #500, #530, and #537
  * `IOBuffer` from #501 and #504
  * `range` with kw args and `LinRange` from #511
  * `cp` and `mv` from #512
  * `indexin` from #515
  * `isuppercase` and friends from #516
  * `dims` and `init` kwargs from #518, #528, #590, #592, and #613
  * `selectdim` from #522 and #531
  * `repeat` from #625
  * `fetch(::Task)` from #549
  * `isletter` from #542
  * `isbitstype` from #560
  * `at-cfunction` from #553 and #566
  * `codeunit` and `thisind` and friends from #573
  * `something` from #562
  * `permutedims` from #582
  * `atan` from #574
  * `split` and `rsplit` from #572
  * `mapslices` from #588
  * `floatmin` and `floatmax` from #607
  * `dropdims` from #618
  * required keyword arguments from #586
  * `CartesianRange` in `at-compat` from #377
  * `finalizer` from #416
  * `readline`, `eachline`, and `readuntil` from #477, #541, and #575
  * curried `isequal`, `==`, and `in` from #517
  * `Some` from #435 and #563
  * `at-warn` and friends from #458

* Remove old deprecations

* Deprecate:
  * `Compat.Sockets` from #545 and #594
  * `TypeUtils` from #304
  * `macros_have_sourceloc` from #355
  * `Compat.Sys` from #380, #433, and #552
  * `Compat.MathConstants` from #401
  * `Compat.Test`, `Compat.SharedArrays`, `Compat.Mmap`, and `Compat.DelimitedFiles` from #404
  * `Compat.Dates` from #413
  * `Compat.Libdl` from #465 (and #467)
  * `AbstractDateTime` from #443
  * `Compat.Printf` from #435
  * `Compat.LinearAlgebra` from #463
  * `Compat.SparseArrays` from #459
  * `Compat.Random` from #460, #601, and #647
  * `Compat.Markdown` from #492
  * `Compat.REPL` from #469
  * `Compat.Serialization` from #473
  * `Compat.Statistics` from #583
  * `Fix2` from #517
  * `Compat.Base64` from #418
  * `Compat.Unicode` from #432 and #507
  * `notnothing` from #435 and #563
  * `Compat.IteratorSize` and `Compat.IteratorEltype` from #451
  * `enable_debug(::Bool)` from #458
  * `Compat.Distributed` from #477
  * `Compat.Pkg` from #485
  * `Compat.InteractiveUtils` from #485
  * `Compat.LibGit2` from #487
  * `Compat.UUIDs` from #490
  * `Compat.qr` from #534
  * `Compat.rmul!` from #546
  * `Compat.norm` abd friends from #577

* Remove obsolete README entry, missed in #385

* Remove obsolete tests (e.g. missed in #372)

* Remove obsolete `VERSION` conditionals and some minor clean-up
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.

4 participants