Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Option additions #57
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
base: main
Are you sure you want to change the base?
Option additions #57
Changes from all commits
e300c0f
2c1d16b
90483c8
f61e47e
79c8086
be9897e
de8a86f
5c06440
a056cd0
29b3d5d
8b198d5
9280051
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Not sure if we shouldn't rather leave the implementation of such a function to e.g. testing frameworks (which might also want to raise a different exception).
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.
This isn't for testing. You should consider using this every time you reach for
getExn
because it gives you a better, easier to track down error message when the assumption that lead you to using it inevitably fails in production 🙃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.
One thing that comes to mind here - do we need an additional alternative where you can pass your own exception? I recall people in the community talking about adding that themselves.
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.
What's the use case for that? To have a distinct exception that they can catch?
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.
Yeah, exactly. But let's ignore it for now, feels peripheral.
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.
Ok, right, so there is definitely demand in the community for something like this.
And what do you think about the naming? Usually we have
exn
in the method name when the function might raise. That wouldn't be the case withexpect
. Maybe it should begetExnWithMessage
?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 think that would ensure that absolutely no-one would pick this over
getExn
, unfortunately 😉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.
expectExn
could be an alternative. Though I don't think the convention is that functions that raise should always useExn
, but rather that it signifies a variant of a function that distinguishes itself by raising. Similar to how not all functions that can return an option usesOpt
.Is there not an annotation that can be put on functions that raise? I thought reanalyze utilized that, but I can't find it now.
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.
Naming this one is hard for sure... I'm trying to come up with a good alternative as well. I like
expect
because I've used it a lot in Rust, but I'm not sure it'd feel very familiar to people who haven't used Rust. I'm ok withgetExnWithMessage
, but I understand if it feels too verbose. So let's maybe think a round or two more about what could be a good alternative name, before we decide.@glennsl correct, it's
@raises
: https://rescript-lang.org/syntax-lookup#raises-decoratorWe should eventually work our way through all the bindings and ensure all of them have
@raises
where appropriate. That'll be a lot of work though, so probably wise to do it little by little, over time. It's also less important than the general documentation is right now.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 logged an issue for
@raises
.Still haven't figured out a better name than
expect
orgetExnWithMessage
. Anyone else got something? Otherwise we'll need to figure out the best choice between those two.