fix: remove fallthrough switch statement #1247
Conversation
| } | ||
| case 'content': { | ||
| if (format === 'content') return result | ||
| break |
There was a problem hiding this comment.
note: I don't think this break statement should have been here. I've kept the equivalent for consistency, by converting to a return, but by breaking here, we'd end up returning undefined instead of throwing, which seems wrong. I'd be happy to open another PR fixing that issue as a follow-up - I'd rather keep the functionality identical with this PR, in case there would be side effects to the fix.
|
@wmhilton any thoughts on this? |
|
@jcubic conflicts are now resolved, this is ready to merge! |
|
🎉 This PR is included in version 1.8.10 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
I've looked at this code more than a few times and always thought to myself it needed to be changed. So I'm in support. FYI, William isn't currently working on the project, so it is best not to mention him. The current maintainers already get notifications when a PR is submitted, so there's really no need to use the @ mention unless you are replying directly to that person. |
// BEHOLD! THE ONLY TIME I'VE EVER WANTED TO USE A CASE STATEMENT WITH FOLLOWTHROUGH!Hahaha. So in the end, even this only time was a mistake! 😂 Good job y'all! |
* chore: fix broken link in README.md (isomorphic-git#1154) * fix(react-native): fix for "<Intermediate Value>.stream is not a function" errors in React Native (isomorphic-git#1156) * chore: remove fallthrough switch statement * docs: add @mmkal as a contributor * docs: add @mmkal as a contributor Co-authored-by: Xavier Francisco <xavier.n.francisco@gmail.com> Co-authored-by: Corbin Crutchley <crutchcorn@gmail.com>
npm run add-contributorand follow the prompts to add yourself to the READMEThis removes the use of a fall-through switch statement. While it technically works, it's confusing to humans - and, as it turns out, to machines.
I have a slightly strange use case, where the runtime I'm using can't handle async/await. I'm able to make isomorphic-git work by supplying it with a filesystem with purely sync operations. But to avoid promises, I'm using babel-plugin-transform-async-to-promises, which converts async/await statements into
.then/.catchwrappers.This allows shimming the global
Promiseobject to do everything synchronously (or throw, in cases like http calls where that's impossible). Unfortunately babel-plugin-transform-async-to-promises can't handle switches with fallthrough. So most likely this is technically a bug with that package rather than isomorphic-git, but fixing it there would be really complicated since it involves dynamic code-generation, and requires very careful thought about edge-cases. Plus, the repo isn't very active. Fixing it here makes the code clearer, IMO and avoids the violation of a lint rule.I haven't added or changed any tests because the functionality should be identical. If the existing tests pass, it should be safe to merge.
cc @wmhilton