-
Notifications
You must be signed in to change notification settings - Fork 89
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
Runner: Allow directories to be passed with trailing slashes/.hrx
#1816
Conversation
Relying on `path.sep` to consistently separate paths is wrong, since un-normalized Windows paths can be separated by either `/` or `\`.
This makes directory addressing transparent across both of these, so passing `spec/expressions/` or `spec/expressions/syntax.hrx` will no longer break the runner.
@@ -164,3 +165,11 @@ export default abstract class SpecDirectory { | |||
return this.path; | |||
} | |||
} | |||
|
|||
/// Splits a relative path into its constituent components. |
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.
/// Splits a relative path into its constituent components. | |
/** Splits a relative path into its constituent components. */ |
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.
For non-exported functions, I'd prefer to use ///
-style comments since they're so much easier to write and edit.
lib-js/spec-directory/spec-path.ts
Outdated
if (path.endsWith('/') || path.endsWith(p.sep)) { | ||
path = path.substring(0, path.length - 1); | ||
} |
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.
a/b/c//
would return a/b/c/
rather than a/b/c
, should this be a while
instead of an if
?
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.
Actually, it should probably just use p.normalize()
first to handle those and other edge cases. Updated.
@@ -137,7 +139,7 @@ export default class VirtualDirectory extends SpecDirectory { | |||
if (this.subdirCache[filename]) { | |||
throw new Error(`${message}: ${filename} is a directory`); | |||
} | |||
if (filename.includes(path.sep)) { | |||
if (filename.includes('/') || filename.includes(path.sep)) { |
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.
It took me a while to realize this is because hrx will always have /
regardless of whether this is unix or windows 🤔
Perhaps a comment would be helpful here and on normalizeSpecPath()
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.
It's actually because /
is a valid path separator on Windows, even outside of HRX files. I'll add a comment.
This makes directory addressing transparent across both of these, so
passing
spec/expressions/
orspec/expressions/syntax.hrx
will no longerbreak the runner.