feat: add EmptyPathSegment, distinct from PathSegment{}

[rvagg]

This might require a little bit more thought than I've put into it so far, but this mainly comes from some code that has var nextSeg datamodel.PathSegment, then using that naively when it's not initialised, for String() in particular. I normally initialise it in a loop using a path.Shift() operation but with Len()==0 I have the zero value. My assumption was that the zero value would just be empty and I could be fine with that. But even if I did a path.Shift() on an empty path, I'd get the same thing. Which is, the zero value of a PathSegment is a 0 index. This is probably reasonable in some situations but doing a nextSeg.String() and seeing "0" was certainly not expected.

There is some awkwardness, but it's not new, here's some variations of awkward things you can do, already and with the new thing here:

[]datamodel.PathSegment{{}, {}, {}}).String()
// "0/0/0"
[]datamodel.PathSegment{datamodel.EmptyPathSegment, datamodel.EmptyPathSegment, datamodel.EmptyPathSegment}).String()
// "//"
[]datamodel.PathSegment{datamodel.PathSegmentOfString(""), datamodel.PathSegmentOfString(""), datamodel.PathSegmentOfString("")}).String()
// "//"

The two big changes in here are:

• Introduction of EmptyPathSegment which is "" and distinct from the zero-value of PathSegment
• Introduction of EmptyPath as a matching type, which is the same as the zero-value of Path
• Return EmptyPathSegment from Path#Shift() and Path#Last() which currently return the zero-value, which is the 0 index.

This does mean you can't path.Last() == PathSegment{}, you'd now have to path.Last() == EmptyPathSegment. But I think that maybe you should be checking the length first anyway. I've put that in the docs.

Which is better?

[]datamodel.PathSegment{datamodel.Path{}.Last(), datamodel.Path{}.Last(), datamodel.Path{}.Last()}).String()
// "0/0/0"

[]datamodel.PathSegment{datamodel.Path{}.Last(), datamodel.Path{}.Last(), datamodel.Path{}.Last()}).String()
// "//"

They both suck, but Last() giving you a "zero-index" is a bit more unhelpful than literal "" I think.

[willscott]

is there a reason to embrace this complexity versus collapsing to a 'canonical' form at the interface?

so, e.g. on all of the examples you provide i would hope to end up getting an empty string from String() and have the behavior of methods act as though any of these are simply an empty path, right?

I think i agree that a 0 is distinct, although that rendering seems incorrect as well - i would not expect []datamodel.PathSegment{{},{}} to go into the first element of an array. It would not in it's equivalent http formulation.

[rvagg]

I'm trying to avoid being too heavy handed here and provide some help rather than fundamentally changing things (too much). These problems are prefigured in the existing docs:

// Using PathSegment's natural zero value directly is discouraged
// (it will act like ParsePathSegment("0"), which likely not what you'd expect).

and on containsString

// If it returns false, it implicitly means "containsInt", as these are the only options.

and this, which gets to the suggestion of collapsing at the Path if you have an empty segment:

(Fun note: Empty strings were originally used for this sentinel,
but it turns out empty strings are valid PathSegment themselves, so!))

I'm actually not sure what an empty string path segment indicates though; perhaps at the data model layer we ought to have this kind of expansive notion but I don't think such data would survive through our codecs. Given that this is already flagged, I'm not keen to litigate this whole point, but rather take a Chesterton's Fence approach of saying that there may be dragons under this.

So the 2 things I want to fix with this:

1. Provide a better return value for Path#Last() and Path#Shift() than something that suggests the segment is 0
2. Make it more clear that the zero value of PathSegment probably isn't what you want; and EmptyPathSegment might be better