pmjones/fig-leaf — pull request #7 by simensen (2013-06-27 06:48:30+00:00)
Title: Logic work.

I had a few ideas that I wanted to try and play with. The biggest of which was a major change in the idea of the logical path prefix and the fully qualified logical path.

Rather than considering the fully qualified logical path as the input, I broke the concept out into its own standalone thing that can be used by other definitions.

What was THE fully qualified logical path before is now called "source" and I added an "output" definition.

I played with the idea of changing the name from logical path prefix to logical path base with wording trying to sway from treating it as if it was *only* mean to a prefix. Instead, it is a string on which to base the transformation. Maybe the wording is wrong and maybe there is a better concept of this, but I think something like this might work.

I changed the definition of the logical path base (previously logical path prefix) to say that it itself can be specified as a fully qualified logical path. This means that it is valid to pass the same value for source (a fully qualified logical path) and logical path base (a fully qualified logical path).

This let me tackle the terminated by / followed by problem in a new way:

> ... the _logical path base_, with a _logical separator_ appended, must exist as a substring from the beginning of the _source_ in order to be considered valid.

So for example, by this definition, the following should be an accurate representation in code:

$source = "/Foo/Bar/Baz";
$logicalPathBase = "/Foo/Bar";
if ($logicalPathBase.'/' !== substr($source, 0, strlen($logicalPathBase.'/'))) {
// invalid!
}

So I think that this makes it clear that we're not saying that the logical path base itself should be terminated by a _logical separator_ but that the string created by appending a _logical separator_ should exist as a part of _source_.

How does this sit with you? :)

I also added in _root path_ definition. That is useful for some of the special handling in the code and in the logical path base definition.

On that note, I accounted for some specific cases. I know you feel that the special cases are not that special, but by treating these two cases as special ( source === logical path base and root === logical path base ) we avoid a lot of the other complicates and limitations we've been running into by trying to not handle them any differently.

I've heavily annotated the code in the test suite to show why each big of code is required and how it relates to the spec. I've also annotated the actual spec some, too, using `>` blockquotes. Those are there to help explain the rules and why I changed them the way I did so we can discuss this with proper context.

I'm not sure how many compromises I made here... but I think most of the code looks the same. :( However, I spent a lot of time working on the spec side of things to try and make the spec and code look the same. There were difficulties in how certain things were written out before so I wanted to try and see if I could fix that.

One thing you might like is that I added something to ensure that a trailing directory separator exists on the fs path once we get to a point int he algorithm where we know for certain that the target path was intended to be a directory. We don't know up until that point, though, and we only know because of all of the additional checks we're doing.

I think that is the key thing. These changes all work together. Taking any one thing out will make parts of it fail / behave in an unexpected way. So I don't think we can cherry pick too many things to fix the spec otherwise.

Because of that, if you have issues with any specific thing, I'd love to talk about them on a case by case basis rather than trying to address some of these changes outside of the PR by trying to move some of the concepts in independently.

I feel like that is what we did yesterday and today and I don't feel like we ended up getting very far. Despite the fact that the code from my first PR actually worked for all of our test cases we ended up with a few test cases that had to be disabled with no apparent path forward.

Meanwhile, we have working code that addresses our concerns but that code requires some changes to the spec to make sure that the two are in sync. I'm hoping I've done a better job of making the spec say what we actually want the code to do in a way that you're comfortable with.

So if we could spend a little time working on tweaking what I've done here to see if we can get it to what you'd be happy with I'd appreciate it!

Oh, the tests all pass in my environment. I enabled `testBSFileAsPrefix` and `testBSDirectory` again as this code works with it. All of the spec changes were made to try and make the spec look like this implementation.