-
Notifications
You must be signed in to change notification settings - Fork 645
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
fix: #2138 linter issues with fail function #2171
fix: #2138 linter issues with fail function #2171
Conversation
- refactoring fail function to be a custom Error class - adjusting throw class given the above changes - adding missing imports - removing unused imports
Thanks @JuanJo4! I'll take a look today or tomorrow. |
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.
Hey @JuanJo4 - I love it. I came around to really liking the new class rather than the function-based approach. Initially, I thought fail
was throw
ing, but I had misremenbered. Since we were throwing whatever gets returned from fail
, I think a new class that extends Error
is perfect.
I found a typo in one of your file names. Would you mind fixing that? Once you do, we can merge this.
Thanks for doing this! Really appreciate it.
__tests__/uitls.test.ts
Outdated
@@ -0,0 +1,20 @@ | |||
import { MstError } from "../src/utils" |
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.
issue: @JuanJo4 - would you mind renaming this file to __tests__/utils.test.ts
? I think the file name is a typo currently.
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.
oh! my bad... fixed.
__tests__/uitls.test.ts
Outdated
@@ -0,0 +1,20 @@ | |||
import { MstError } from "../src/utils" | |||
|
|||
describe("MstError custom error class", () => { |
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.
praise: @JuanJo4 - thank you for these test! Make sense to me!
Perfect! Merging. I'll get a new 6.0.0 release out soon. We're in a preview mode for the next major and this should land there once we ship it. Thanks for the help, and please let us know if there's anything else you'd like to work on. |
What does this PR do and why?
It solves #2138, this has happened before (#2137 & #1989) and was solved only for those specific cases, this PR aims to fix once and for all.
This is what I did:
fail
function and created a newMstError
class that extends fromError
.fail
function accordingly.Steps to validate locally
I've added some basic unit tests, not sure if that's enough.