Dirname and basename - Githubissues

joshgoebel / wren-console

A CLI/repl tool for Wren written in pure Wren (aMaP)

MIT License

23 stars 6 forks source link

Dirname and basename #16

Closed glennj closed 3 years ago

glennj commented 3 years ago

Looking into the build failures...

... Ah, tests can't be conditional. I'll rewrite those.

glennj commented 3 years ago

the resolver Wren kernel (and it's Path class)

Well I see dirname is already implemented. I see 2 choices here:

move basename into Path
have File.dirname wrap Path.new().dirname

joshgoebel commented 3 years ago

Well I see dirname is already implemented. I see 2 choices here:

I'm reaching out to the Wren team again to talk about the future... Path is another thing that only exists in Console but not CLI... and if we care about upstreaming patches then just depending on Path would require it also be made part of the public API (it's not currently, it's used internally by cli and resolver modules) and also upstreamed.

joshgoebel commented 3 years ago

You don't need the tests to be so receptive actually, the test scaffold has no idea WHICH line generate output, only the total output plan has to match the output generated... so you can move the expectations up to the initial declarations and then do the testing in a loop.

joshgoebel commented 3 years ago

If you're happy with this I will rebase and merge.

glennj commented 3 years ago

Yes, thumbs up.

joshgoebel commented 3 years ago

if (Platform.isWindows) {
  tests = tests.map {|t|
    t["given"]    = t["given"].replace("/", "\\\\")
    t["expect"] = t["expect"].replace("/", "\\")
    return t
  }.toList
}

Why \\\\? Tests are failing but only on windows...

glennj commented 3 years ago

I forgot about that. The reveal of Path.dirname kinda took the steam out of my sails for this one.

I was fumbling around with different approaches to getting windows tests working, then I realized I shouldn't be using github for my docker testing, then life intervened and my progress stopped.

I was contemplating closing this PR and taking a fresh approach to it. Maybe put it as Draft until the windows bugs are ironed out.

joshgoebel commented 3 years ago

I was contemplating closing this PR and taking a fresh approach to it.

We're still limited by Path being "off limits" though because of official CLI considerations. Hence my jumping back on this and seeing if we can push it to home as-is.

Boom, all tests green. Not sure why you felt the double was needed at all since you're using a single in the line right below?

Unless you can think of something I'm missing I think this is good to merge now.