Skip to content

Add panic function - #72

Merged
zth merged 5 commits into
rescript-lang:mainfrom
glennsl:feat/panic
Mar 1, 2023
Merged

Add panic function#72
zth merged 5 commits into
rescript-lang:mainfrom
glennsl:feat/panic

Conversation

@glennsl

@glennslglennsl commented Feb 25, 2023

Copy link
Copy Markdown
Contributor

panic is like failwith but raises a native JavaScript exception that gives a better stack trace and in-browser debugging experience.

cknitt
cknitt reviewed Feb 26, 2023
View reviewed changes
Comment threadsrc/Core__Error.res Outdated
Comment threadsrc/Core__Error.resi Outdated
Comment threadsrc/Core__Error.resi Outdated
@zth

zth commented Feb 26, 2023•
edited
Loading

Copy link
Copy Markdown
Member

I typically don't use the exn functions, but is it problematic that we're changing what exn a bunch of functions is raising? I'm thinking about the case when people are actually relying on catching them.

Edit: oops, accidentally fat finger closed the PR...

@zthzth closed this Feb 26, 2023
@zthzth reopened this Feb 26, 2023
@glennsl

glennsl commented Feb 26, 2023

Copy link
Copy Markdown
ContributorAuthor

It should probably be noted as a breaking change, but these are all functions that have non-exception-raising alternatives. It doesn't really make sense to catch these exceptions, since you should then use the alternative non-exception-raising function instead (that definitely doesn't mean that people aren't still using it though).

aspeddro
aspeddro reviewed Feb 27, 2023
View reviewed changes
Comment threadsrc/Core__List.res Outdated
@zth

zth commented Feb 28, 2023

Copy link
Copy Markdown
Member

So, what about splitting this up, first adding the panic function, and then doing a separate discussion/PR on whether the current exn methods should use panic under the hood?

zth
zth reviewed Feb 28, 2023
View reviewed changes
Comment threadsrc/Core__Error.resi Outdated
@glennsl
feat(error): add panic/Error.panic
8c06a60
refactor(error): less magic needed
Co-authored-by: Christoph Knittel <christoph@knittel.cc>
docs(error): panic fixes
Co-authored-by: Christoph Knittel <christoph@knittel.cc>
docs(error): ReScript, not OCaml
@glennsl
test(error): test panic
cb56e44
@glennsl
docs(changelog): update changelog for panic/Error.panic
56886f0
@glennslglennsl mentioned this pull request Feb 28, 2023
Open
@glennsl

glennsl commented Feb 28, 2023

Copy link
Copy Markdown
ContributorAuthor

So, what about splitting this up, first adding the panic function, and then doing a separate discussion/PR on whether the current exn methods should use panic under the hood?

Done. Internal use split out to #79

zth
zth approved these changes Mar 1, 2023
View reviewed changes

@zthzth left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff! @cknitt ?

cknitt
cknitt reviewed Mar 1, 2023
View reviewed changes
Comment threadCHANGELOG.md Outdated
glennsland others added 2 commits March 1, 2023 12:41
@zth
zth merged commit d7eac7f into rescript-lang:mainMar 1, 2023
@zth

zth commented Mar 1, 2023

Copy link
Copy Markdown
Member

Thanks @glennsl !

@glennsl
glennsl deleted the feat/panic branch March 1, 2023 17:58
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cknittcknittcknitt left review comments
@zthzthzth approved these changes
+1 more reviewer
@aspeddroaspeddroaspeddro left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@glennsl@zth@cknitt@aspeddro