-
Notifications
You must be signed in to change notification settings - Fork 36
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
feat: add mkdir command #34
Conversation
It seems like you're not using the default dprint config ?, but https://github.com/dsherret/dax/blob/main/dprint.json doesn't have any configuration |
dprint leaves a lot of code as-is, so I think maybe something else is formatting the files while you're working on them? The default width for dprint is 120 characters for example, so it seems like something else is formatting the files. Would you be able to revert all formatting changes with git? |
I'll align dprint with deno fmt after we merge this PR. I'm using it because there's also rust in this project. Edit: I went ahead and merged it because it was quiet minimal #37 |
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.
Thanks for adding all these commands, @sigmaSd! We're almost there on this one.
mod.test.ts
Outdated
@@ -736,3 +736,33 @@ Deno.test("test remove", async () => { | |||
await $`rm -r ${nonEmptyDir}`; | |||
assertEquals($.fs.existsSync(nonEmptyDir), false); | |||
}); | |||
|
|||
Deno.test("test mkdir", async () => { | |||
const dir = Deno.makeTempDirSync(); |
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.
In #40 I just added a withTempDir helper. Let's use that here to ensure that these temporary directories get cleaned up after the test is run.
src/commands/mkdir.ts
Outdated
} | ||
} | ||
|
||
interface mkdirFlags { |
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.
Let's change this to start with a capital: MkdirFlags
mod.test.ts
Outdated
.then( | ||
(r) => r.stderr, | ||
); | ||
const expecteError = Deno.build.os === "windows" |
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.
Nit, typo: expectedError
src/utils.ts
Outdated
@@ -0,0 +1,15 @@ | |||
export async function stat( |
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.
Maybe call this lstat
since it's using Deno.lstat
and not Deno.stat
?
src/utils.ts
Outdated
) { | ||
try { | ||
const info = await Deno.lstat(path); | ||
return test(info); |
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.
Maybe we should just return Deno.FileInfo | undefined
from this function? Then we could do a stat call once in the code above. Right now we're doing possibly two stat calls per path instead of max 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.
I added the other suggestions, but I don't get this one, how can lstat happen twice ?
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 I see your point, can you merge this as is (if there are no other issues), and I'll do the changes in the other pr
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.
Sure.
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.
Looks great. Thanks!
I run
drpint fmt
still same issue, is there something I need to do ?