@johnjansen as soon as specs pass I am ok with this. Note that the history shall be squashed. I could handle via github directly, unless, you prefer to do that on your own.

@bcardiff ill let you do the merge, and happy to see all the attempts squashed into oblivion ;-)

@bcardiff is it just a matter of clicking "close and comment" to merge the PR ?

@johnjansen Thank you for this!

A question I had for some time now: why two empty strings are returned when the match is not found? Can we maybe return nil in this case? What's the use case for retuning two empty strings? I ask because I'm pretty sure there must be a reason.

Hi @asterite

I wondered the same, but fell back to the same behavior as ruby ... much as i'm enthusiastic to get my first contribution merged, this probably warrants more discussion. I cant formulate a good argument either way without contradicting myself in the process :-)

J

TL;DR 1: I think is ok to return a non-nilable tuple

I am not sure if rpartition needs to return nil. If we try to use it for cycling, it sounds like returning nil seems reasonable, to stop iteration. But due to the nature of unpacking the result using it in a loop the code looks weird:

s = "abc.def.ghi.jkl"

# if rpartition returned nil
while m = s.rpartition "."
  s, _, suffix = m
  suffix # => "jkl" , "ghi" , "def"
end

s # => "abc"

I would expect a block alternative to be used to cycle the results. #each_rpartition(Char|String, &block) or something like that that will yield all the partitions.

And last, for keeping the non-nillable tuple, a partition always exists. if you concat the 3 parts you get the original string. If the method was named "match" or something like that I would agree that we need a nil to represent the no-matching result.

TL;DR 2: I think #partition and #rpartition should be equal when there is no match

Maybe if no string is matched the result could be {self, "", ""} so the rpartition is used to split the right part after the separator. Like extracting the extension of a file, when no extension is found, getting an empty string seems appropriate. Also that would mean that #partition and #rpartition return the same result when there is no match at all (i would like that). But all this would defer from ruby, I am sure they had some reason for this. NB: currently there is no String#partition.

I'll write partition too, but it will be later in the week ... if you want to ticket it and assign it to me :-)

adding a block version of both could be a third task and we could agree on the nilability of elements ... once again, happy to pick that up too

@johnjansen 👍 on #partition for later. But I would say to no worries for the block version. I am not sure if it would be that useful.

awesome @bcardiff i will put it up on my whiteboard ;-)

In the future (or soon) we could support unpacking in if and while. Ruby 2.3 or 2.4 supports this already. Basically:

if x, y = foo
end

is the same as this:

tmp = foo
if tmp
  x, y = tmp
end

So returning nil in partition would be good.

ok @asterite i'll get to it in a little bit ... also note the partition PR ;-)

@johnjansen In any case I don't know what final shape I'd like it to be. I'd like to know real use cases for partition before making a choice. What's your use case?

i dont have one right now, but someone was complaining about its absence, and i wanted to start contributing with something simple, so i decided to have a go

@asterite ... and now i do ... in the process of porting some code, i found this

    def may_shift_colon?
      return false unless @text.includes?(":")
      partitions = @text.partition(":")
      partitions.last[0] !~ /\A\d+/ || partitions.first[-1] !~ /\A\d+/
    end

obviously, im happy to handle this any way thats necessary, but its one example ;-)

thinking about an implementation of the above with a union type return as below

class String
  def partition(search : (Char | String)) : Tuple((String|Nil), (String|Nil), (String|Nil))
    pre, mid, post = {nil, nil, nil}
    search_size = search.is_a?(Char) ? 1 : search.size
    case pos = self.index(search)
    when .nil?
      pre = self
    when 0
      mid = search.to_s
      post = self[(pos + search_size)..-1]
    else
      pre = self[0..(pos - 1)]
      mid = search.to_s
      post = self[(pos + search_size)..-1]
    end
    {pre, mid, post}
  end
end

def may_shift_colon?(text)
    return false unless text.includes?(':')
    first, _, last = text.partition(':')
    (last && last[0] !~ /\A\d+/) || (first && first[-1] !~ /\A\d+/)
end

presumably the last line can be

  last.not_nil![0] !~ /\A\d+/ || first.not_nil![-1] !~ /\A\d+/

given that, i like the guarantee that each part is a string ... although i can see the use for Nil too

I found this about a use case for partition:

https://m0j0.wordpress.com/2007/07/11/partition-saves-my-bacon/

So it seems Python has this too. It's also a bit more efficient than split because it doesn't allocate an array. I think we can add it and keep the same behaviour as in Ruby and Python. rpatition also works the same in Ruby and Python: two empty strings followed by the original string.

I think the reason behind that behaviour is this. Imagine we expect to receive a string with a colon and we want to get what's before and after the first colon. So:

"foo:bar".partition(":") # => {"foo", ":", "bar"}
"foo:bar:baz".partition(":") # => ["foo", ":", "bar:baz"}

Now if there's no colon, we assume it's at the end of the string (because we are searching from the start). So:

# think "foo:".rpartition(":") # => {"foo", ":", ""}
"foo".partition(":") # => {"foo", "", ""}

An empty string is returned instead of the separator to distinguish it from the case where the colon was actually there.

Now, if we want to do the same but for the last colon:

"foo:bar".rpartition(":")     # => {"foo", ":", "bar"}
"foo:bar:baz".rpartition(":") # => {"foo:bar", ":", "baz"}

If there's no colon, since we are searching from the back of the string, we assume it's at the beginning of the string. So:

# think ":foo".rpatition(":") # => {"", ":", "foo"}
"foo".rpartition(":") # => {"", "", "foo"}

So now I think it makes a lot of sense to do it like that :-)

@asterite It's also useful for splitting file paths, i.e. getting last path segment, that is the filename.

@johnjansen Could you rebase this PR against master?

@asterite @bcardiff i think we are good to merge ... ?

@johnjansen I'm not sure, as there are still listed 19 commits for the changes.

https://github.com/crystal-lang/crystal/pull/3429/commits

I might recommend you look at the Contributing document for instructions to perform a clean rebase.

thanks @luislavena, i hate rebase ;-) always messes with my head

@johnjansen Don't worry, I also hate it too :-)

I actually tried to manually rebase this PR but failed... or, mmm... I'm lazy to rewrite history.

Another alternative you can do is to start from scratch: close this PR and create a new one in a different branch.

@johnjansen I use rebase all the time, but for some reason you're getting other commits that shouldn't be part of this branch.

I recommend taking a look to the document I linked before about updating your local copy against the upstream master and ensure only your commits are in the list of the rebase.

Perhaps use the interactive mode? (git rebase -i upstream/master)

@asterite im going to close this one, and open another, with just the stuff we want :-)