Skip to content
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

[FES] Replace throw error with FES #6098

Merged
merged 1 commit into from
Apr 21, 2023
Merged

Conversation

chechenxu
Copy link
Contributor

@chechenxu chechenxu commented Apr 4, 2023

Resolves part of #5649

Changes:

Replace some throw error with friendly error system message.

PR Checklist

@welcome
Copy link

welcome bot commented Apr 4, 2023

🎉 Thanks for opening this pull request! Please check out our contributing guidelines if you haven't already. And be sure to add yourself to the list of contributors on the readme page!

@limzykenneth
Copy link
Member

The original issue only recommended replacing console.warn and console.error with FES, functions throwing errors should not usually have the throwing behaviour entirely replaced with FES messages because it will change the overall behaviour of the code. When throwing an error in JS it will halt current execution where as simply logging a message (including with the FES), code execution can continue which in the case where it should have halted may cause undefined behaviour.

@chechenxu
Copy link
Contributor Author

@limzykenneth Thanks for your feedback. Will update this PR!

@chechenxu
Copy link
Contributor Author

Hi @limzykenneth , please take another look and let me know whether the current changes look good to you. Thanks!

@limzykenneth
Copy link
Member

I'm ok with this but @davepagurek you may want to have a look just in case since it only changes messages in WebGL side of things. Maybe the messages can be rephrased in some cases as well but we can possibly do this in another PR. Do merge if it looks good to you @davepagurek.

Copy link
Contributor

@davepagurek davepagurek left a comment

Choose a reason for hiding this comment

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

This looks good to me!

@davepagurek davepagurek merged commit 1316bcc into processing:main Apr 21, 2023
@processing processing deleted a comment from allcontributors bot Apr 21, 2023
@davepagurek
Copy link
Contributor

@all-contributors please add @chechenxu for code

@allcontributors
Copy link
Contributor

@davepagurek

I've put up a pull request to add @chechenxu! 🎉

@chechenxu
Copy link
Contributor Author

@davepagurek @limzykenneth Thank you!

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.

3 participants