Separate AbstractString interface from iteration protocol #26133

Keno · 2018-02-20T20:53:11Z

Up until now, the basic interface new AbstractStrings had to implement was:

struct MyString; ...; end
next(::MyString, i::Int64)::Tuple{Char, Int64}
isvalid(::MyString, i::Int64)::Bool
ncodeunits(::MyString)::Int64

In this interface, the iteration state (i.e. the second tuple element returned
from next) always had to be the next valid string index. This is inconvenient
for several reasons:

The iteration protocol will change, breaking every use of this API
Some strings may want iteration states other than linear indicies for efficiency
reasons (e.g. RopeStrings)
Strings implementors can no longer assume that the second argument they receive
was necessarily produced by them, so may need to do various validation of the
iteration sate on every iteration.

This PR attempts to remedy this, by introducing a new generic LeadIndPairs iterator.
This iterator behaves similarly to Pairs, except that instead of the index of an
element, it gives the index of the next element (in other words, the indicies lead
the values by one element). The astute reader will note that the elements of this
iterator are precisely the elements of the tuple currently returned by next.
Thus, this PR changes, the requisite method to implement from:

next(::MyString, i::Int64)::Tuple{Char, Int64}

to

next(::StringLIPairs{MyString}, state::Any)::Tuple{Pair{Int, Char}, Int64}

where StringLIPairs{T} = LeadIndPairs{Char, Int, EachIndexString{T}, T}

Efficient implementations of iteration over strings, the indicies as well as Pairs
can be derived from this iterator. The reason this iterator is useful is perhaps best
understood by considering strings to be variable-length encodings of character arrays.
In a variable-length encoding, one generally decodes the value and the length (i.e. the
index of the next element) at the same time, so it makes sense to base the API
on the implementation of an iterator with these semantics.

To demonstrate the use and test the new abstract implementations based on this iterator,
there are three string types in the test suite:

CharString, as before, which simply wraps an array of Chars with direct indexing. The
only change to this iterator is to change the signature of the next method.
RopeString, which strings together several Strings, and more importantly does not have
efficient linear iteration state.
DecodeString, which decodes escape sequences on the fly as part of iteration. This string
type demonstrates one string type wrapping another string type to test the interface from
both sides

This bootstraps and many of the abstract string operations work on the three test string types I mentioned. Some more cleanup and testing is required, but this seemed like the right time to stop for feedback.

Keno · 2018-02-20T21:02:24Z

One more thing to note is that is may actually make sense to not deprecate the use of next(s::AbstractString, i::Int64) from the user side with the new iteration protocol. Since we're reclaiming that name, we could have a generic definition like:

next(itr, i) = ((a, b) = first(leadindpairs(itr, i)); (b, a))

that would generically work (and give the (val, nextind)) tuple for everything, not just strings.
We now have significantly better ways to do generic string processing (with Stateful, iterating
over pairs, etc), but without a good upgrade path for next(::AbstractString, i::Int64) I worry the deprecation would be a bit jarring, since that is used all over the place in the ecosystem (our fault, since base started it).

StefanKarpinski · 2018-02-20T22:07:38Z

Bikeshedding the name LeadIndPairs:

Next
NextPairs
LaggedPairs

stevengj · 2018-02-20T22:15:31Z

I thought strings had to implement nextind and prevind if they want to expose index iteration?

Keno · 2018-02-20T22:17:31Z

No, we have generic versions of those based on isvalid:

julia/base/strings/basic.jl

Line 462 in 2e1fb5e

function nextind(s::AbstractString, i::Int, n::Int)

Keno · 2018-02-21T20:22:49Z

I kinda like Next.

samoconnor · 2018-02-23T04:26:05Z

DecodeString, which decodes escape sequences on the fly as part of iteration

I have a LazyJSON.String <: AbstractString like this.
https://github.com/samoconnor/LazyJSON.jl/blob/master/src/AbstractString.jl#L38

There should be a test case that ensures that it is efficient to do comparison of a dynamically un-escaped string respecting the SizeUnknown trait. You want to be sure that the iterator only goes as far as needed for the == comparison to fail, and that we're not accidentally calling length from some generic string function. Maybe put invalid escape sequences in the test string, and check that the == returns false before erroring on the bad escape.

RopeString, which strings together several Strings

I'd like to use something like this for splice-editing JSON text
https://discourse.julialang.org/t/announce-a-different-way-to-read-json-data-lazyjson-jl/9046/9
https://discourse.julialang.org/t/ann-stringbuilders-jl/9223/3

I now see that this used to be in base but was removed: #12330
Is it time for this to be reincarnated in some part of the standard library?

IOString

Another variation on AbstractString is here : https://github.com/samoconnor/LazyJSON.jl/blob/master/src/IOStrings.jl

This is a string that grows in size as data is received from an IO channel. In my current implementation, when the parser hits the end, it throws a ParseError, which causes more data to be read from the IO channel, then the parser resumes. Maybe it would be interesting to consider something like this as a test case. It might be better if blocking to wait for more data was in the next function (but in my use case this wouldn't work because the parser works with pointer). <ABitMad>I can just about imagine doing something like this with userfaultfd. (ok, google says someone already did an AWS S3 mmap thing )</ABitMad>.

Keno · 2018-02-23T20:27:08Z

@nanosoldier runbenchmarks(ALL, vs=":master")

nanosoldier · 2018-02-23T20:58:51Z

Something went wrong when running your job:

NanosoldierError: failed to run benchmarks against primary commit: failed process: Process(`sudo cset shield -e su nanosoldier -- -c ./benchscript.sh`, ProcessExited(1)) [1]

Logs and partial data can be found here
cc @ararslan

Keno · 2018-02-23T21:00:41Z

:( cc @ararslan

ararslan · 2018-02-24T00:01:04Z

That should be fixed by #26186.

Keno · 2018-02-24T19:45:35Z

@nanosoldier runbenchmarks(ALL, vs=":master")

Keno · 2018-02-24T19:45:56Z

@ararslan kick nanosoldier for me?

samoconnor · 2018-02-25T10:34:17Z

we have generic versions of those based on isvalid:

Related: #26202

Keno · 2018-02-25T20:34:02Z

I haven't verified, but from your description, I think this should fix #26202, because the abstract pairs method will be properly derived from the generic method.

Keno · 2018-02-25T21:04:51Z

Maybe today?:
@nanosoldier runbenchmarks(ALL, vs=":master")

nanosoldier · 2018-02-26T02:57:02Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

Keno · 2018-02-26T05:20:30Z

Ok, that's decently encouraging. The only string-performance-sensitive regression I see is join (we have a bunch particularly in dates and the micro benchmarks). join is a notoriously unreliable benchmark (it benchmarks a bit of an unrealistic case, with a very large separator), I believe due to memory allocation patterns, but I'll benchmark locally. My plan is to rename this to Next, do some cleanup and then get this merged.

Up until now, the basic interface new AbstractStrings had to implement was: ``` struct MyString; ...; end next(::MyString, i::Int64)::Tuple{Char, Int64} isvalid(::MyString, i::Int64)::Bool ncodeunits(::MyString)::Int64 ``` In this interface, the iteration state (i.e. the second tuple element returned from `next`) always had to be the next valid string index. This is inconvenient for several reasons: 1. The iteration protocol will change, breaking every use of this API 2. Some strings may want iteration states other than linear indicies for efficiency reasons (e.g. RopeStrings) 3. Strings implementors can no longer assume that the second argument they receive was necessarily produced by them, so may need to do various validation of the iteration sate on every iteration. This PR attempts to remidy this, by introducing a new generic `Next` iterator. The iterator is defined to iterate (values, next index) tuple, which is the return value the `next` method on strings at the moment and thus allows for a natural transition from the older API. Thus, this PR changes, the requisite method to implement from: ``` next(::MyString, i::Int)::Tuple{Char, Int} ``` to ``` next(::StringNext{MyString}, state::Any)::Tuple{Tuple{Char, Int}, Any} ``` where `StringNext{T} = Next{T, EachIndexString{T}}` Efficient implementations of iteration over strings, the indicies as well as `Pairs` can be derived from this iterator. The reason this iterator is useful is perhaps best understood by considering strings to be variable-length encodings of character arrays. In a variable-length encoding, one generally decodes the value and the length (i.e. the index of the next element) at the same time, so it makes sense to base the API on the implementation of an iterator with these semantics. To demonstrate the use and test the new abstract implementations based on this iterator, there are three string types in the test suite: - CharString, as before, which simply wraps an array of `Chars` with direct indexing. The only change to this iterator is to change the signature of the `next` method. - RopeString, which strings together several Strings, and more importantly does not have efficient linear iteration state. - DecodeString, which decodes escape sequences on the fly as part of iteration. This string type demonstrates one string type wrapping another string type to test the interface from both sides

Keno · 2018-02-27T00:18:37Z

Renamed LeadIndPairs to Next (and reversed the order of the returned tuple).
I think this is essentially in mergeable state now. I'll take a look at join performance, since that was flagged by nanosoldier, but as I mentioned that benchmark is a frequent false-positive offender, so there might be nothing to do here.

Keno · 2018-02-27T00:34:14Z

Local benchmark shows no difference in join performance.

samoconnor · 2018-02-27T09:05:46Z

base/strings/iteration.jl

+StringNext(x::T, idx) where {T<:AbstractString} = Next(x, idx)
+StringNext(x::T, idx, itr) where {T<:AbstractString} = Next(x, idx, itr)
+
+start(sp::StringNext) = 1


Should it be start(sp::StringNext) = firstindex(sp.data) ?
The doc says that string indexing starts at 1, but there are other places that use firstindex (same for first(::EachStringIndex) = 1 above).

The more places the 1 is not hard coded, the easier it'll be to eventually support AbstractString types with non standard indexing.

Part of the goal of this change is to separate the assumption currently baked into the string code that indices and itartion state of strings are the same thing. We want indices to always be 1 through ncodeunits(s) but allow iteration state to be anything at all (even non-integers). So firstindex(s) and start(s) should be totally unrelated: the former is always 1 but the latter can be anything at all.

start(s) here is just a default (custom string types can override it). It is also going away with the iteration protocol change. I do want to eventually define non-standard indexing for strings, but that's a natural extension of this and string should always support linear indicies.

We want indices to always be 1 through ncodeunits(s)

I've been trying out various string implementations as a way to understand what the abstract interface contract is. If firstindex(s) == 1 is part of the protocol, so be it.

Is it also to be assumed that isvalid(s, firstindex(s)) == true? If this wasn't required that would allow "padding" of the codeunit space in cases where that is more efficient.

I have a SplicedString implementation where the indexes are in the range 1:ncodeunits(s) (and isvalid(s, 1) == true), but where the index space is only sparsely populated.
It all works nicely for constant time indexing and iteration, but anyone who writes a i = i+1 loop is going to find it take a long time to run. Is this too heretical?

julia> s = LazyJSON.SplicedString("Foo", " ", "Bar") "Foo Bar" julia> [keys(s)...] 7-element Array{Int64,1}: 1 2 3 1099511627777 2199023255553 2199023255554 2199023255555 julia> [(i >> 40, i & (2^40-1)) for i in keys(s)] 7-element Array{Tuple{Int64,Int64},1}: (0, 1) (0, 2) (0, 3) (1, 1) (2, 1) (2, 2) (2, 3)

firstindex(s) == 1

I'd be fine relaxing this constraint, but I don't care too much.

isvalid(s, firstindex(s)) == true

This one I do care about.

Is this too heretical?

I mean, you can do various kinds of encoding here, this one included, but I'd rather go the direction where strings can have non-standard index kinds, but all still need to support linear indexing no matter how slow it is.

StefanKarpinski

I'm not so keen on this, I'm afraid. Overriding next(StringNext{MyString}, state) seems very convoluted as a way to tell people to define iteration for their custom string types. The fact that the definition seems to typically involved returning the same index value twice seems problematic as well both stylistically, and potentially for performance, although I suppose in any case that's not too slow anyway that duplication will be inlined and eliminated anyway.

StefanKarpinski · 2018-02-27T11:55:14Z

base/iterators.jl

+The first value returned by the `Next` iterator should correspond to the element at `idx`.
+Please note that if you override iteration for `Next{A}` and your iteration state is not
+the next index, you will have to additionally overload `Next(data::A, idx, itr::I)` for
+four `A`.


What does "for four A" mean here?

StefanKarpinski · 2018-02-27T11:56:28Z

base/iterators.jl

+('a', 2)
+
+julia> first(Next(['a','b','c'], 3))
+('c', 4)


This seems like a somewhat confusing example. Would a more typical usage example be possible?

what confuses you about it? The purpose of the second example was to show that the last tuple will have an out-of-bounds index. Would adding first(Next(['a','b','c'], 2)) before this one be better?

StefanKarpinski · 2018-02-27T12:00:20Z

base/strings/iteration.jl

+StringNext(x::T, idx) where {T<:AbstractString} = Next(x, idx)
+StringNext(x::T, idx, itr) where {T<:AbstractString} = Next(x, idx, itr)
+
+start(sp::StringNext) = 1


Part of the goal of this change is to separate the assumption currently baked into the string code that indices and itartion state of strings are the same thing. We want indices to always be 1 through ncodeunits(s) but allow iteration state to be anything at all (even non-integers). So firstindex(s) and start(s) should be totally unrelated: the former is always 1 but the latter can be anything at all.

Keno · 2018-02-27T15:25:35Z

The fact that the definition seems to typically involved returning the same index value twice seems problematic as well both stylistically

I mean that's hardly surprising, because that was the only option at the moment. Also, looking back at this, I realized SubString should use the parent's index state rather than duplicating the index - will fix. Some of this will change with the new iteration protocol anyway. Separating the indexing and iteration state is a new feature. The RopeString example I added shows a kind of string where this is exploited.

, and potentially for performance, although I suppose in any case that's not too slow anyway that duplication will be inlined and eliminated anyway.

Yes, where this is the case, the compiler will eliminate it. Registers are also fairly cheap during return if it doesn't.

stevengj · 2018-02-27T17:48:13Z

I'm still confused by what problem this PR is solving. Shouldn't we just say that the iteration state i for next(string, i) is arbitrary (not necessarily an index), and that you should use nextind etc if you want to work with indices?

i.e. string implementations should define (at least):

isvalid(::MyString, i::Integer), getindex(::MyString, i::Integer), firstindex (if ≠ 1), and lastindex for indexing. Optionally nextind and prevind if this can be done more efficiently than the fallback using isvalid.
codeunit and ncodeunits for raw code-unit access
Optionally start(::MyString)::SomeType, next(::MyString, i::SomeType), done(::MyString, i::SomeType) for iteration, but have a fallback definition that uses indexing and nextind.

(Analogous to how array implementations define indexing, and get iteration for free.)

Keno · 2018-02-27T17:56:55Z

I'm still confused by what problem this PR is solving. Shouldn't we just say that the iteration state i for next(string, i) is arbitrary (not necessarily an index), and that you should use nextind etc if you want to work with indices?

Yes, that is one possible solution, but it's unfortunately 2x slower for cases where you need both (because getting the next index generally involves the same work as decoding the character). That's a decently common operation because things like search want to return indices. This solves that problem by making getting both the basic operation from which the others are derived.

stevengj · 2018-02-27T18:04:59Z

Yes, that is one possible solution, but it's unfortunately 2x slower for cases where you need both

If you want to optimize that, couldn't you implement a Pairs iterator?

That is, the most basic string types implement just indexing. Optimized string types also implement a custom Pairs{IndexLinear, MyString} iterators rather than the fallback. This way you push the additional complexity only onto performance fanatics and not onto everyone implementing string types (which, admittedly, is probably not that many people).

Also, this way people wanting both the indices and the values use the standard pairs API rather than something string-specific.

Keno · 2018-02-27T18:11:45Z

If you want to optimize that, couldn't you implement a Pairs iterator?

That's what I thought, but Pairs is the wrong operation, because it gets the index at the start of the character, but what you want is the index at the end of the character (the Next iterator is similar though, which is why I called it NextIndPairs originally). I do also think that this is simpler once you get used to it, because by implementing this one method efficiently, you automatically get efficient iterations of a) strings b) indicies c) pairs as well as efficient getindex rather than having to implement each yourself. The derivations don't really work in the any of the other directions.

Keno · 2018-02-27T19:46:25Z

@StefanKarpinski Would you be happier if we fell back to assuming the iteration state was the same as the string state:

function iterate(x::StringNext{<:AbstractString}, state)
    y = iterate(x, state)
    y === nothing && return nothing
    (c, idx) = y
    (c, idx), idx
end

We'd have to be a bit careful about circular fallbacks, but that would allow defining string iterate on string and having this work automatically.

samoconnor · 2018-02-27T22:31:49Z

That's a decently common operation because things like search want to return indices.

👍
A parser wants to traverse a string using an efficient iterator, but return structures containing real indices.

@StefanKarpinski

Clarify that `firstindex(str)` should always be `1` for any `AbstractString`, as mentioned by @StefanKarpinski [here](#26133 (comment)). Also reference `prevind` and `eachindex`. Also introduce the "code unit" terminology and mention the `codeunit` functions.

samoconnor · 2018-02-28T22:55:43Z

We want indices to always be 1 through ncodeunits(s) but allow iteration state to be anything at all (even non-integers).

Sounds good. The iteration state might be a tuple of tree node and local index. Or the iteration state might be an opaque handle from an external system.

[getting both char and next index is] a decently common operation because things like search want to return indices. This solves that problem by making getting both the basic operation from which the others are derived.

This sounds good too, but doesn't it assume that iteration state and index are the same?

Maybe the API should have something like this:

index(s, iteration_state)::Integer
start(s, i::Integer=firstindex(s)) -> iteration_state

A parser that is blindly iterating along and finds an interesting delimiter can call index(s, state) to get an index. And the consumer of the parser's output can call start(s, i) to start iterating from that index.

By default:

index(::AbstractString, state::Integer) = state

But specialiseable like:

function index(t::TreeString, state::Tuple{TreeStringNode, Integer})
    n, i = state
    return length(n.left) + index(t, (n.parent,1)) + i
end

vtjnash · 2018-03-01T21:21:53Z

I think I like nextpair (NextPairs), as a corollary to the existing nextind ("next index") and as the duals of the pairs/indices/values functions.

Keno · 2018-03-01T21:24:00Z

nextpair isn't quite right, because, it's not the same as the next pair. It's the index of the next pair with the value of the current one.

StefanKarpinski · 2018-03-02T19:41:01Z

base/iterators.jl

@@ -1070,6 +1124,7 @@ end
 function fixpoint_iter_type(itrT::Type, valT::Type, stateT::Type)
    nextvalstate = Base._return_type(next, Tuple{itrT, stateT})
    nextvalstate <: Tuple{Any, Any} || return Any
+    nextvalstate === Union{} && return Union{}


Mostly? Prevents things from erroring in the wrong place if you mess up how to do iteration.

StefanKarpinski · 2018-03-02T19:42:07Z

base/strings/iteration.jl

@@ -0,0 +1,120 @@
+# A specialized iterator for EachIndex of strings
+struct EachStringIndex{T<:AbstractString}
+    s::T


Somewhat tangential, but while we're moving this type, let's spell the field out as .string instead of .s.

Can we have nextindex spelled out instead of nextind too? 🚲🏚

StefanKarpinski · 2018-03-02T19:48:29Z

base/strings/iteration.jl

+end
+
+# Derive iteration over pairs from `StringNext`
+const StringPairs{T<:AbstractString} = Iterators.Pairs{Int, Char, EachStringIndex{T}, T}


According to @Keno this is:

the kind of Pairs iterator you get when you call pairs on a string of type T

There should probably be a comment to that effect.

Can it be written something like const StringPairs{T<:AbstractString} = typeof(pairs(T()) (I had this problem creating a const type alias for view(Vector{UInt8}), the type is really convoluted, so I ended up with const nobytes = UInt8[]; const ByteView = typeof(view(nobytes))

No, I don't think that will work since there is no actual T at the time of definition. The RHS needs to use parametric type syntax to work – despite the appearance, this is not a normal assignment, it's what used to be written as typealias.

Keno · 2018-03-05T19:52:27Z

Talking with @JeffBezanson and @StefanKarpinski we will punt on generalizing string iteration state for 1.0. In particular, we will:

Continue to require iteration state to be codeunit indicies.
Implement a special deprecation for next of AbstractStrings to keep that working, but tell people to use iterate (And require that firstindex(s) is a valid iteration state argument for iterate on strings). Not that is may not be possible to do proper deprectations for both sides of this interface, in which case we will prefer to break the string implementer side of the interface, rather than the string user one.
Keep the Next iterator from this branch, naming to be decided and in which direction it generalizes (iteration state or indexing).
Keep the basis of string reverse iteration as Reverse{StringPairs{S}} and don't make assumptions on the iteration state (as on this branch)
In 1.x, introduce a more general kind of abstract string (@StefanKarpinski proposed AbstractStringLike - another option might be AbstractCharSequence) that does not assume iteration states are codeunit indices (but still uses a join index and iteration state).

I will do 1/2 in the iteration PR and once that's in open separate PRs for 3/4. We also discussed whether we would need to do something about SubString in light of 5, since such a generalized string type generally wants to index with parent indices, but concluded that it would be too weird for e.g. find on a simple substring to return parent indicies, so either we'll have to have a different view type for the generalized AbstractString, or have integer indices be inconsistent.

Keno · 2018-03-05T22:30:35Z

Let's keep the branch around for a bit, it's useful as a reference.

@StefanKarpinski

Clarify that `firstindex(str)` should always be `1` for any `AbstractString`, as mentioned by @StefanKarpinski [here](#26133 (comment)). Also reference `prevind` and `eachindex`. Also introduce the "code unit" terminology and mention the `codeunit` functions.

@StefanKarpinski

Clarify that `firstindex(str)` should always be `1` for any `AbstractString`, as mentioned by @StefanKarpinski [here](#26133 (comment)). Also reference `prevind` and `eachindex`. Also introduce the "code unit" terminology and mention the `codeunit` functions. (cherry picked from commit 3b6773d)

Keno requested review from StefanKarpinski and JeffBezanson February 20, 2018 20:53

Keno added this to the 1.0 milestone Feb 21, 2018

Keno force-pushed the kf/abstractstringit branch from 676e0e7 to 941c1c5 Compare February 21, 2018 21:26

Keno force-pushed the kf/abstractstringit branch from 941c1c5 to 74961ab Compare February 23, 2018 20:08

Keno force-pushed the kf/abstractstringit branch from 74961ab to 6d9fb91 Compare February 24, 2018 19:42

Keno force-pushed the kf/abstractstringit branch from 6d9fb91 to 03448ed Compare February 25, 2018 22:36

Keno force-pushed the kf/abstractstringit branch from 03448ed to c2658e1 Compare February 27, 2018 00:15

Keno changed the title ~~WIP/RFC: Separate AbstractString interface from iteration protocol~~ Separate AbstractString interface from iteration protocol Feb 27, 2018

samoconnor reviewed Feb 27, 2018

View reviewed changes

StefanKarpinski reviewed Feb 27, 2018

View reviewed changes

stevengj mentioned this pull request Feb 28, 2018

bugfixes for rsplit (for unicode and empty strings) #26238

Merged

stevengj mentioned this pull request Feb 28, 2018

string doc clarifications #26259

Closed

Keno added the triage This should be discussed on a triage call label Mar 1, 2018

StefanKarpinski reviewed Mar 2, 2018

View reviewed changes

Keno closed this Mar 5, 2018

ararslan deleted the kf/abstractstringit branch March 5, 2018 19:54

Keno restored the kf/abstractstringit branch March 5, 2018 22:30

Keno mentioned this pull request Mar 5, 2018

Iteration protocol change #25261

Merged

3 tasks

JeffBezanson removed the triage This should be discussed on a triage call label Mar 6, 2018

DilumAluthge deleted the kf/abstractstringit branch March 25, 2021 22:06

Separate AbstractString interface from iteration protocol #26133

Separate AbstractString interface from iteration protocol #26133

Conversation

Keno commented Feb 20, 2018 • edited by simonbyrne Loading

Keno commented Feb 20, 2018

StefanKarpinski commented Feb 20, 2018

stevengj commented Feb 20, 2018

Keno commented Feb 20, 2018 • edited Loading

Keno commented Feb 21, 2018

samoconnor commented Feb 23, 2018

Keno commented Feb 23, 2018

nanosoldier commented Feb 23, 2018

Keno commented Feb 23, 2018

ararslan commented Feb 24, 2018

Keno commented Feb 24, 2018

Keno commented Feb 24, 2018

samoconnor commented Feb 25, 2018

Keno commented Feb 25, 2018

Keno commented Feb 25, 2018

nanosoldier commented Feb 26, 2018

Keno commented Feb 26, 2018

Keno commented Feb 27, 2018

Keno commented Feb 27, 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

StefanKarpinski left a 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

Choose a reason for hiding this comment

Keno commented Feb 27, 2018

stevengj commented Feb 27, 2018 • edited Loading

Keno commented Feb 27, 2018

stevengj commented Feb 27, 2018 • edited Loading

Keno commented Feb 27, 2018 • edited Loading

Keno commented Feb 27, 2018

samoconnor commented Feb 27, 2018

samoconnor commented Feb 28, 2018

vtjnash commented Mar 1, 2018

Keno commented Mar 1, 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

StefanKarpinski Mar 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Keno commented Mar 5, 2018

Keno commented Mar 5, 2018

Keno commented Feb 20, 2018 •

edited by simonbyrne

Loading

Keno commented Feb 20, 2018 •

edited

Loading

stevengj commented Feb 27, 2018 •

edited

Loading

stevengj commented Feb 27, 2018 •

edited

Loading

Keno commented Feb 27, 2018 •

edited

Loading

StefanKarpinski Mar 2, 2018 •

edited

Loading