Skip to content

Fix builtin exit() #1413

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Sep 26, 2019
Merged

Fix builtin exit() #1413

merged 2 commits into from
Sep 26, 2019

Conversation

ChJR
Copy link
Contributor

@ChJR ChJR commented Sep 24, 2019 •
edited
Loading

After #1343, RustPython could not provide builtin exit() properly.

youknowone
youknowone reviewed Sep 24, 2019
View reviewed changes
tests/snippets/exit.py
@@ -0,0 +1,4 @@
from testutils import assert_raises
Copy link
Member

Choose a reason for hiding this comment

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

by convention, tests/snippets/builtin_exit.py looks better name of this test file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this file(exit.py) can hold sys.exit() test. Would be better if separating them?

@youknowone
Copy link
Member

I have no idea how this breaking is related to #1343

But the patch looks legit to me

@coolreader18
Copy link
Member

coolreader18 commented Sep 25, 2019 •
edited
Loading

This looks good, thanks! Could you edit handle_exception in main.rs to check if the exception is an instance of SystemExit and if so exit with the appropriate error code/print its args?

@ChJR
Copy link
Contributor Author

ChJR commented Sep 25, 2019 •
edited
Loading

@coolreader18 I am working what you suggested. Thanks to your guidance.

@ChJR ChJR closed this Sep 25, 2019
@ChJR ChJR reopened this Sep 25, 2019
@ChJR ChJR changed the title Fix builtin exit() Fix exit() Sep 25, 2019
@ChJR
Copy link
Contributor Author

ChJR commented Sep 25, 2019 •
edited
Loading

I thought builtin exit() and sys.exit() need to placed in separate PRs. But they are related each other. I would handle them in this PR.

This PR has already merged into master. I would make another PR.

@youknowone youknowone merged commit f209929 into RustPython:master Sep 26, 2019
@ChJR ChJR changed the title Fix exit() Fix builtin exit() Sep 26, 2019
@ChJR ChJR deleted the hotfix/exit branch September 26, 2019 13:29
@ChJR ChJR mentioned this pull request Oct 6, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@youknowone youknowone youknowone approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ChJR @youknowone @coolreader18