-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dirname and basename #16
Conversation
src/module/io.wren
Outdated
return dirname == "" ? separator : dirname | ||
} | ||
|
||
static lastIndexOf(string, char) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This belongs in essentials, eventually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, but currently it's problematic for Wren Console to have dependencies on Essentials since theoretically it should be compilable without as well... (and we're still not sure how closely we'll work with the Wren CLI project - where we can't assume Essentials is present).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we move this to a CoreExtra
class/module (open to naming suggestions) for keeping track of such utilities that can't [yet] be moved to essentials? There is an issue in Wren repo for adding this to core Wren, but no progress on it.
Looking into the build failures... ... Ah, tests can't be conditional. I'll rewrite those. |
6aeb39a
to
d1d8df3
Compare
Well I see dirname is already implemented. I see 2 choices here:
|
test/io/file/basename.wren
Outdated
}.toList | ||
} | ||
|
||
// tediously, we need to spell out the tests one-by-one |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, technically with wren-console
you could have just written this with Testie. But if we ever hope to upstream it to Wren CLI
(which seems possible in this case) then yeah you're stuck with this. It's super annoying.
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 |
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. |
If you're happy with this I will rebase and merge. |
Yes, thumbs up. |
9fd271b
to
a238cf1
Compare
Why |
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. |
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. |
No description provided.