-
Notifications
You must be signed in to change notification settings - Fork 634
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
suggestion: code style about async try catch #525
Comments
Awesome! The return type of |
it's a bit too non-standard for me. The standard library should avoid some magic-hacks. Not the same as
Golang only require to handle |
@axetroy It doesn't mean exposing |
Not so long ago we had similar syntax to this example:
It was deemed non-idiomatic TypeScript and I agree that Check #472 for more details. |
In fact, this style of interface return is not impossible. But limited to some specific interfaces const [byte, error] = await copy(writer, reader);
// byte: record how many bytes have been copied
// error: Whether an error has occurred, including promise reject. In this scenario, you need to accurately get the number of bytes of copy, even if the entire copy fails. Unfortunately, |
In my opinion, no every It is used in somewhere "make sense" like |
You could always use the native promise methods: const mode = await stat(fn).catch(() => null);
const items = await Deno.readDir(dir).catch(() => Deno.mkdir(dir, true).then(() => []));
export async function ensureDir(dir: string): Promise<void> {
let pathExists = false;
const stat = await Deno.stat(dir).catch(() => await Deno.mkdir(dir, true));
if (stat && !stat.isDirectory()) {
throw new Error(
`Ensure path exists, expected 'dir', got '${getFileInfoType(stat)}'`
);
}
} |
Really looks like Haskell's This may be used in other modules but not standard. |
I do think we still want to stick with idiomatic JS try-catch, but I feel this wrapping Also maybe we could implement |
I think @kevinkassimo put it well. For std it's best to s itick with idiomatic JS, but it would be interesting to see experiments with other methods. |
@ry The reason that prompted me to open this issue is to see the following code in std let mode = null;
try {
mode = (await stat(fn)).mode;
} catch (e) {} I think this code should be avoided in std |
@Dreamacro Yeah I agree this looks strange. At lease I should have just do The idea is that for this single case the error is not important and we just want to leave it as null if an error occurs. But in many other cases you do want to handle the error with the try-catch block, which is idiomatic JS. If we were to really implement something to make this easier here, I would suggest some helper like const statInfo = await maybe(stat(fn)); // yields FileInfo | null
const mode = statInfo ? statInfo.mode : null;
// I do even want to use optional chaining once it is approved by TC39,
// so that I could do just `const mode = (await maybe(stat(fn)))?.mode;` We could implement this in If you believe these helpers are very helpful, would be great if you could contribute and submit a PR to add these to |
Yeah, we should omit the |
ESlint would yell if not used. |
No it doesn't. The binding was not optional until recently, so it will just allow |
This PR applies various fixes, cleanups and improvements to Stripe logic.
Why I suggest this code style
I have a quick look at
deno_std
and find out that the "try-catch" code style could be improved. I use a little trick about "try-catch" to improve code structure in the real world.it's helpful when we want to throw a wrapper error and reduce indent deep to make code clear. btw, it seems like golang error handle 😛.
Example
https://github.com/denoland/deno_std/blob/master/http/file_server.ts#L163
before
after
https://github.com/denoland/deno_std/blob/master/fs/empty_dir.ts#L8
before
after
https://github.com/denoland/deno_std/blob/master/fs/ensure_dir.ts#L7
before
after
Bad part
The text was updated successfully, but these errors were encountered: