makes slash operator default lhs value to an empty string

[erikkrieg]

I was looking for a good first issue and came across #1298

The change is that the parser will now default an empty string literal on the left hand side of the / operator if it fails unwrapping a value for the lhs. This makes it possible to create a path with an expression like: / "Users" / user.

Lemme know if there is anything that should be changed!

Edit:
The approach was changed to wrapping the left-hand side (lhs) value of a join expression (using the / operator) to an Option.

[erikkrieg]

Hey, @casey. Anything I am missing or should do to make this ready for review?

[casey]

Sorry for the delay! I was out of town for the weekend, and just now catching up to my GitHub notifications. Thanks for taking this on! I left some comments on the code. The unwrap_or will might leave the parser in a bad state, so best to avoid it, and I think it's cleaner to make lhs an Option instead of the empty string.

I'd also like to see a few more tests. For example, in what context could the unwrap_or cause problems? Is there a test that would have caught it?

[erikkrieg]

Thanks for the feedback! I'll take a look tonight.

[erikkrieg]

I went on quite the journey trying to make lhs an optional expression for Join before putting a pin in that approach and going back to something close to my first pass. I went through the following files while changing lhs to an option:

• src/assignment_resolver.rs
• src/evaluator.rs
• src/expression.rs
• src/node.rs
• src/parser.rs
• src/string_kind.rs
• src/summary.rs
• src/variables.rs

The number of places to make the change wasn't so much a problem (tho a bit messy, I think) but the real issue was that I hit a snag with lifetimes on a borrow in src/variables.rs when needing to push a ref to a boxed expression onto the stack. In the case where lhs.is_none() I needed to provide an expression containing a string literal and I couldn't figure out a way to create one in a way that satisfied the lifetime requirements.

So I came back around full circle, except without the potentially breaking unwrap on parse_value.

[casey]

How about this:

        // if `lhs` is `Some`:
        Expression::Concatenation { lhs, rhs } | Expression::Join { Some(lhs), rhs } => {
          self.stack.push(rhs);
          self.stack.push(lhs);
        }
       // If `lhs` is `None`:
        Expression::Join { None, rhs } => {
          self.stack.push(rhs);
        }

Variables is an iterator that must return all variables (and thus traverse all sub-expressions) in an expression. If lhs is None, it contains no variables, so nothing needs to be pushed onto the stack.

[erikkrieg]

Okay, your suggestion for variables makes sense. I have an another branch locally with those changes, so I can give that a shot and see if that was truly the last compiler error for the option approach.

The question, then, is do you prefer the None case being handled in all those modules downstream of the parser? Or creating an empty string literal expression at the parser and leaving the rest unchanged?

[casey]

I like handling the None case better, it seems more explicit.

[casey]

Added some more comments. Also, some tests using prefix / in different contexts would be good.

[erikkrieg]

Added some more comments. Also, some tests using prefix / in different contexts would be good.

I added tests for when lhs is None that seem to mirror the pre-existing tests for the slash operator. I also added a test for the error case when rhs values/expression is not provided. Are there any other tests besides what are in the slash_operator test module that I should be looking at?

[casey]

Can pub(crate) be removed now?

[casey]

These look good! And the PR looks good too, I think just a couple of pub(crate)s that aren't necessary.

It's really interesting that now // "foo" is valid, and is basically "" / "" / "foo".

I'm not sure if that should be valid syntax, the thought being that maybe in the future, we want to use // for something else, and if we allow // "foo" now, then using // for something else would be a breaking change.

If, god forbid, I ever have to implement support for arithmetic in Just, then it would be pretty natural to use // for division. Also, I think it's pretty uncommon to want double slashes, although I suppose you could do: "https:" // "ordinals.com" / "page" to build a URL. Also, windows has UNC paths, which start with //, so you could use // there.

What do you think?

Either way, let's not hold this PR up. Even if we decide to make // illegal, we can merge this PR, and I can add a PR that changes // into an error, as long as I do it before I cut a release.

[erikkrieg]

That's a good question...

TL;DR: I think //"foo" should be a valid expression.

Without having very intimate knowledge of Just's internals, here are a few ideas for arithmetic:

1. Treat operators, like +, * and /, differently for ints and floats so that arithmetic can be written with a syntax familiar to many other popular languages.
2. Use functions for arithmetic, like div(4,2), add(1,2), mul(2,2). It is more verbose, but very transparent.
3. Wrap expressions to indicate arithmetic should be performed, i.e. [ 4 / 2 ] works but 4 / 2 does not.

I am a less in favour of using // for division because I feel it is more likely to lead to confusion writing and reading math equations in Justfiles compared to the other approaches. But because I am not sure how much effort each of these options might take, I am not really considering that dimension in my ranking.

Now, that doesn't answer you main question, but I think it might be enough to consider reserving // not worthwhile enough to affect the decision much either way. So then the question maybe become whether it is useful to have //"foo" evaluate to "//foo". I suppose the simple answer is "yes", and you gave a couple cases. It also seems a bit more intuitive to me, but then again, I have been looking at this feature for a few days, so take that with a heap of salt.

[casey]

That all sounds pretty reasonable. Also, if we have arithmetic, we'll also have static typing, so that could be used to disambiguate joins from division.

I just merged master into this branch, and assuming the tests pass, is this ready to merge?

[erikkrieg]

is this ready to merge?

Yes, it is!

Appreciate all the feedback and patience.

[casey]

Sorry for the delay! Just added some docs. Thanks for sticking with this, this is a nice improvement.