Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Add panic function#72

Merged
zth merged 5 commits intorescript-lang:mainrescript-lang/rescript-core:mainfrom
glennsl:feat/panicglennsl/rescript-core:feat/panicCopy head branch name to clipboard
Mar 1, 2023
Merged

Add panic function#72
zth merged 5 commits intorescript-lang:mainrescript-lang/rescript-core:mainfrom
glennsl:feat/panicglennsl/rescript-core:feat/panicCopy head branch name to clipboard

Conversation

@glennsl
Copy link
Contributor

@glennsl glennsl commented Feb 25, 2023

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

src/Core__Error.res Outdated Show resolved Hide resolved
src/Core__Error.resi Outdated Show resolved Hide resolved
src/Core__Error.resi Outdated Show resolved Hide resolved
@zth
Copy link
Member

zth commented Feb 26, 2023

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...

@zth zth closed this Feb 26, 2023
@zth zth reopened this Feb 26, 2023
@glennsl
Copy link
Contributor Author

glennsl commented Feb 26, 2023

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).

src/Core__List.res Outdated Show resolved Hide resolved
@zth
Copy link
Member

zth commented Feb 28, 2023

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?

src/Core__Error.resi Outdated Show resolved Hide resolved
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
Copy link
Contributor Author

glennsl commented Feb 28, 2023

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

Copy link
Member

@zth zth left a comment

Choose a reason for hiding this comment

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

Good stuff! @cknitt ?

CHANGELOG.md Outdated Show resolved Hide resolved
glennsl and others added 2 commits March 1, 2023 12:41
@zth zth merged commit d7eac7f into rescript-lang:main Mar 1, 2023
@zth
Copy link
Member

zth commented Mar 1, 2023

Thanks @glennsl !

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.