-
-
Notifications
You must be signed in to change notification settings - Fork 117
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
Rename split function keyword argument from keep to keepempty #572
Conversation
src/Compat.jl
Outdated
# https://github.com/JuliaLang/julia/pull/26647 | ||
@static if VERSION < v"0.7.0-DEV.4724" | ||
rsplit(s::AbstractString; limit::Integer=0, keepempty::Bool=false) = | ||
Base.rsplit(s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this have ; limit=limit, keep=keepempty
, too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first I did it. I rebased it so I can't show the results, but it throws MethodError
Test threw an exception of type MethodError
Expression: Compat.split("a b c"; keepempty=true) == ["a", "b", "c"]
MethodError: no method matching split(::String; limit=0, keep=true)
Closest candidates are:
split(::AbstractString) at strings/util.jl:302 got unsupported keyword arguments "limit", "keep"
split(::T<:AbstractString, ::Any; limit, keep) where T<:AbstractString at strings/util.jl:277
split(::T<:SubString, ::Any; limit, keep) where T<:SubString at strings/util.jl:253
Stacktrace:
[1] (::Compat.#kw##split)(::Array{Any,1}, ::Compat.#split, ::String) at ./<missing>:0
[2] include_from_node1(::String) at ./loading.jl:576
[3] include(::String) at ./sysimg.jl:14
[4] process_options(::Base.JLOptions) at ./client.jl:305
[5] _start() at ./client.jl:371
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well... either call Base.split(s, Base._default_delims)
or not define this method at all. But silently doing the wrong thing for limit != 0
or keepempty = false
seems pretty bad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, you are right. I fixed it.
src/Compat.jl
Outdated
rsplit(s::AbstractString, splitter; limit::Integer=0, keepempty::Bool=false) = | ||
Base.rsplit(s, splitter; limit=limit, keep=keepempty) | ||
split(s::AbstractString; limit::Integer=0, keepempty::Bool=false) = | ||
Base.split(s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
test/runtests.jl
Outdated
@test Compat.split(str, ".:."; keepempty=false) == ["a","ba.",".cba",":.dcba"] | ||
@test Compat.split(str, r"\.(:\.)+"; keepempty=false) == ["a","ba.",".cba","dcba"] | ||
@test Compat.split(str, r"\.+:\.+"; keepempty=false) == ["a","ba","cba",":.dcba"] | ||
@test Compat.rsplit(str, ".:."; keepempty=false) == ["a","ba.",".cba.:","dcba"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to test keepempty=true
, as well as setting a limit
, both for the case with and without a splitter
given.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I just copied tests from Julia repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do the methods without the splitter
argument work now? They're still not tested and IIUC, they failed on 0.6. Please either add tests, or if they don't work, we might also just not include them in Compat.
Also, needs a README entry.
@test Compat.split(str, r"\.+:\.+"; keepempty=true) == ["a","ba","cba",":.dcba",""] | ||
@test Compat.split(str, r"\.+:\.+"; limit=3, keepempty=false) == ["a","ba","cba.:.:.dcba.:."] | ||
@test Compat.split(str, r"\.+:\.+"; limit=3, keepempty=true) == ["a","ba","cba.:.:.dcba.:."] | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This nicely covers the methods with splitter
, but doesn't test the ones without.
Ping @appleparan — needs a rebase and test coverage for methods that don't pass |
I decided not to include non- One more thing. I rebased all of my commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I followed review. Is it okay now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm seeing a commit "thisind, 3-arg length/nextind/prevind, codeunit(s) (#573)" here. Some problem with a rebase?
OMG. I did a huge mistake on rebase. Sorry. I will fix it |
0e60346
to
8e24edf
Compare
@appleparan, I think the concern of @martinholters was that you have no tests for the one-argument |
@stevengj Yes, it was. By the way, in Julia v0.6.3,
In my first version, I forced to use keyword argument Summary: I did not include non- |
README.md
Outdated
@@ -415,6 +415,8 @@ Currently, the `@compat` macro supports the following syntaxes: | |||
* `isupper`, `islower`, `ucfirst` and `lcfirst` are now `isuppercase`, `islowercase`, | |||
`uppercasefirst` and `lowercasefirst` ([#26442]). | |||
|
|||
* `keep` keyword argument in `split` and `rsplit` is now `keepempty` ([#26647]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should point out that you have to use Compat.split
and Compat.rsplit
.
Yes, this doesn't provide the complete functionality of 0.7, but it does provide the new API for the old functionality, which should suffice for packages to upgrade. |
I will rebase it. |
CI failures look unrelated, and it has passed on two of the 0.7 runs, so this should be good to go. Barring objections, I'll merge this later today. |
Finally! Thanks! EDIT CI issue have been reported. JuliaLang/julia#27667 |
I don't understand why you can't define split(s::AbstractString; limit::Integer=0, keepempty::Bool=false) =
Base.split(s, @static(isdefined(Base,:_default_delims) ? Base._default_delims : isspace); limit=limit, keep=keepempty) on 0.6 |
On 0.6,
If |
The whole point of this package is to allow you to use the 0.7 API in 0.6. |
I misunderstood. You are right. I will add additional code and tests |
* 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
fix #571