Rename split function keyword argument from keep to keepempty #572

appleparan · 2018-06-10T13:25:40Z

martinholters · 2018-06-11T13:01:49Z

src/Compat.jl

+# 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)


Shouldn't this have ; limit=limit, keep=keepempty, too?

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

https://travis-ci.org/appleparan/Compat.jl/jobs/390385912

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.

yes, you are right. I fixed it.

martinholters · 2018-06-11T13:01:56Z

src/Compat.jl

+    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)


martinholters · 2018-06-11T13:04:36Z

test/runtests.jl

+    @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"]


Would be nice to test keepempty=true, as well as setting a limit, both for the case with and without a splitter given.

Ok. I just copied tests from Julia repo.

martinholters

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.

martinholters · 2018-06-12T06:31:28Z

test/runtests.jl

+    @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


This nicely covers the methods with splitter, but doesn't test the ones without.

stevengj · 2018-06-15T14:52:04Z

Ping @appleparan — needs a rebase and test coverage for methods that don't pass splitter.

appleparan · 2018-06-16T00:28:20Z

I decided not to include non-splitter version, because split/rsplit doesn't have keep keyword argument on 0.6 if not using splitter.
https://github.com/JuliaLang/julia/blob/v0.6.3/base/strings/util.jl#L302

One more thing. I rebased all of my commit.

appleparan

I followed review. Is it okay now?

martinholters

I'm seeing a commit "thisind, 3-arg length/nextind/prevind, codeunit(s) (#573)" here. Some problem with a rebase?

appleparan · 2018-06-18T06:31:14Z

OMG. I did a huge mistake on rebase. Sorry. I will fix it

stevengj · 2018-06-19T20:08:16Z

@appleparan, I think the concern of @martinholters was that you have no tests for the one-argument split function (which defaults to splitting on whitespace).

appleparan · 2018-06-19T21:22:59Z

@stevengj Yes, it was.

By the way, in Julia v0.6.3, split function without splitter just use default delimiter and keyword arguments, limit and keep. Moreover, rsplit doesn't have non-splitter version (commented).

split(str::AbstractString) = split(str, _default_delims; limit=0, keep=false)
#rsplit(str::AbstractString) = rsplit(str, _default_delims, 0, false)

https://github.com/JuliaLang/julia/blob/d55cadc350d426a95fd967121ba77494d08364c8/base/strings/util.jl#L302

In my first version, I forced to use keyword argument keepempty whatever the argument was (with splitter or without splitter). That makes tests fail because original split function without splitter didn't accept limit and keep explicitly. @martinholters pointed this and gave me two choices, one for not including non-splitter ver. and another for forcing to use default argument. I decided to follow the first one.

Summary: I did not include non-splitter version and didn't write tests for them.

martinholters · 2018-06-20T06:05:33Z

README.md

@@ -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])


This should point out that you have to use Compat.split and Compat.rsplit.

martinholters · 2018-06-20T06:07:21Z

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.

appleparan · 2018-06-20T07:37:52Z

I will rebase it.

…mpty`

martinholters · 2018-06-20T09:42:50Z

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.

appleparan · 2018-06-20T11:26:27Z

Finally! Thanks!

EDIT CI issue have been reported. JuliaLang/julia#27667
expect to be fixed soon

stevengj · 2018-06-20T15:09:11Z

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

appleparan · 2018-06-20T15:44:36Z

On 0.6, split without splitter isn't defined to accept keyword argument. It just accept one positional argument only, s::AbstractString.

split(str::AbstractString) = split(str, _default_delims; limit=0, keep=false)

If Compat supports keyword argument split without splitter even 0.6 doesn't, I thought it's nonsense. It feels like out of objective of this package. That's why I didn't define even if I could.

stevengj · 2018-06-20T17:18:17Z

It feels like out of objective of this package.

The whole point of this package is to allow you to use the 0.7 API in 0.6.

appleparan · 2018-06-21T01:29:02Z

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

martinholters reviewed Jun 11, 2018

View reviewed changes

appleparan mentioned this pull request Jun 12, 2018

0.7 compatibility (mostly) JuliaLang/IJulia.jl#623

Closed

martinholters requested changes Jun 12, 2018

View reviewed changes

appleparan force-pushed the ap/split branch from 298c6c2 to 69717bb Compare June 15, 2018 23:57

appleparan changed the title ~~Rename split function keyword from keep to keepempty~~ Rename split function keyword argument from keep to keepempty Jun 16, 2018

appleparan commented Jun 18, 2018

View reviewed changes

martinholters reviewed Jun 18, 2018

View reviewed changes

appleparan force-pushed the ap/split branch 2 times, most recently from 0e60346 to 8e24edf Compare June 18, 2018 09:35

martinholters reviewed Jun 20, 2018

View reviewed changes

martinholters approved these changes Jun 20, 2018

View reviewed changes

Rename split/rsplit function keyword argument from keep to `keepe…

af827d2

…mpty`

appleparan force-pushed the ap/split branch from 2bdd116 to af827d2 Compare June 20, 2018 07:38

martinholters merged commit a9b9972 into JuliaLang:master Jun 20, 2018

martinholters added a commit that referenced this pull request Oct 8, 2019

Drop compat code for split and rsplit from #572

18d693a

martinholters added a commit that referenced this pull request Oct 8, 2019

Drop compat code for split and rsplit from #572

3585e74

martinholters added a commit that referenced this pull request Oct 8, 2019

Drop compat code for split and rsplit from #572

a990ee8

martinholters added a commit that referenced this pull request Oct 8, 2019

Drop compat code for split and rsplit from #572

9de49e6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename split function keyword argument from keep to keepempty #572

Rename split function keyword argument from keep to keepempty #572

appleparan commented Jun 10, 2018

martinholters Jun 11, 2018

appleparan Jun 11, 2018

martinholters Jun 11, 2018

appleparan Jun 11, 2018

martinholters Jun 11, 2018

martinholters Jun 11, 2018

appleparan Jun 11, 2018 •

edited

Loading

martinholters left a comment

martinholters Jun 12, 2018

stevengj commented Jun 15, 2018

appleparan commented Jun 16, 2018 •

edited

Loading

appleparan left a comment

martinholters left a comment

appleparan commented Jun 18, 2018 •

edited

Loading

stevengj commented Jun 19, 2018

appleparan commented Jun 19, 2018 •

edited

Loading

martinholters Jun 20, 2018

martinholters commented Jun 20, 2018

appleparan commented Jun 20, 2018

martinholters commented Jun 20, 2018

appleparan commented Jun 20, 2018 •

edited

Loading

stevengj commented Jun 20, 2018 •

edited

Loading

appleparan commented Jun 20, 2018 •

edited

Loading

stevengj commented Jun 20, 2018

appleparan commented Jun 21, 2018

Rename split function keyword argument from keep to keepempty #572

Rename split function keyword argument from keep to keepempty #572

Conversation

appleparan commented Jun 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

appleparan Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

martinholters left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevengj commented Jun 15, 2018

appleparan commented Jun 16, 2018 • edited Loading

appleparan left a comment

Choose a reason for hiding this comment

martinholters left a comment

Choose a reason for hiding this comment

appleparan commented Jun 18, 2018 • edited Loading

stevengj commented Jun 19, 2018

appleparan commented Jun 19, 2018 • edited Loading

Choose a reason for hiding this comment

martinholters commented Jun 20, 2018

appleparan commented Jun 20, 2018

martinholters commented Jun 20, 2018

appleparan commented Jun 20, 2018 • edited Loading

stevengj commented Jun 20, 2018 • edited Loading

appleparan commented Jun 20, 2018 • edited Loading

stevengj commented Jun 20, 2018

appleparan commented Jun 21, 2018

appleparan Jun 11, 2018 •

edited

Loading

appleparan commented Jun 16, 2018 •

edited

Loading

appleparan commented Jun 18, 2018 •

edited

Loading

appleparan commented Jun 19, 2018 •

edited

Loading

appleparan commented Jun 20, 2018 •

edited

Loading

stevengj commented Jun 20, 2018 •

edited

Loading

appleparan commented Jun 20, 2018 •

edited

Loading