Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Drop ColorPrinter's workaround for BasicObject #1051

Merged
merged 1 commit into from
Jan 12, 2025
Merged

Drop ColorPrinter's workaround for BasicObject #1051

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ jobs:
with:
ruby-version: ${{ matrix.ruby }}
bundler-cache: true
# Added to make Ruby 2.7 correctly require installed default gems, like `pp`.
rubygems: latest
- name: Run tests
run: bundle exec rake test
- name: Run tests in isolation
Expand Down
1 change: 1 addition & 0 deletions irb.gemspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,4 +42,5 @@ Gem::Specification.new do |spec|

spec.add_dependency "reline", ">= 0.4.2"
spec.add_dependency "rdoc", ">= 4.0.0"
spec.add_dependency "pp", ">= 0.6.0"
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to specify this instead of adding it just to Gemfile for testing?
User can install bug fixed version of pp if they want (but I know user usually don't update pp nor add pp to Gemfile)
It is beneficial, but adding it is not a usual case. We don't usually bump reline or rdoc requirements for each bug fixes.
What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

We need to specify it otherwise with the workaround removed, users with newer IRB but older pp will get a regression when dealing with basic objects.

It is beneficial, but adding it is not a usual case. We don't usually bump reline or rdoc requirements for each bug fixes.

We do when we have to make corresponding changes on IRB side?

It's like when repl_type_completor adopts new Prism APIs/structures, it bumps its requirement for prism.

We can of course make the patch conditional based on the version of pp it uses, but IMO with the size of the current patch it's not worth it.

end
5 changes: 0 additions & 5 deletions lib/irb/color_printer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,6 @@

module IRB
class ColorPrinter < ::PP
METHOD_RESPOND_TO = Object.instance_method(:respond_to?)
METHOD_INSPECT = Object.instance_method(:inspect)

class << self
def pp(obj, out = $>, width = screen_width)
q = ColorPrinter.new(out, width)
Expand All @@ -28,8 +25,6 @@ def pp(obj)
if String === obj
# Avoid calling Ruby 2.4+ String#pretty_print that splits a string by "\n"
text(obj.inspect)
elsif !METHOD_RESPOND_TO.bind(obj).call(:inspect)
text(METHOD_INSPECT.bind(obj).call)
else
super
end
Expand Down
Loading