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.

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

Avoid loading files' local variables [Bug #17623] #206

Merged
merged 1 commit into from
Sep 9, 2021
Merged

Avoid loading files' local variables [Bug #17623] #206

merged 1 commit into from
Sep 9, 2021

Conversation

marcandre
Copy link
Member

@marcandre marcandre commented Mar 18, 2021 •
edited by eregon
Loading

@marcandre marcandre requested a review from aycabta March 18, 2021 13:48
@marcandre
Copy link
Member Author

This also fixes the same issue with irb --single that must have existed for years

jeremyevans
jeremyevans approved these changes May 20, 2021
View reviewed changes
Copy link
Contributor

@jeremyevans jeremyevans left a comment

Choose a reason for hiding this comment

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

I think this is a good way to fix the issue.

@eregon
Copy link
Member

eregon commented May 21, 2021

This seems fine to me too given it assigns to IRB::TOPLEVEL_BINDING and does not override ::TOPLEVEL_BINDING.

aycabta
aycabta requested changes Jul 5, 2021
View reviewed changes
exe/irb
@@ -8,4 +8,5 @@

require "irb"

IRB::TOPLEVEL_BINDING = binding
Copy link
Member

Choose a reason for hiding this comment

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

I believe that this is a good solution to fix the problem but this assignment is so far from lib/irb/workspace.rb. Would you move the assignment to the top-level of lib/irb/workspace.rb? And, TOPLEVEL_BINDING is confusing with the existing constant, so I think a different name would be better.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, a different constant name would be much less confusing.

@pocke pocke mentioned this pull request Jul 22, 2021
Closed
@aycabta aycabta merged commit a5c3f6b into ruby:master Sep 9, 2021
@aycabta aycabta mentioned this pull request Sep 9, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eregon eregon eregon left review comments

@aycabta aycabta aycabta requested changes

@jeremyevans jeremyevans jeremyevans approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@marcandre @eregon @jeremyevans @aycabta