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

Drop ColorPrinter's workaround for BasicObject #1051

Merged
merged 1 commit into from
Jan 12, 2025

Conversation

st0012
Copy link
Member

@st0012 st0012 commented Dec 18, 2024

pp 0.6.0+ includes ruby/pp#26 to handle BasicObject, so we can drop the workaround.

(pp supports Ruby 2.7+ too)

@st0012 st0012 marked this pull request as draft December 18, 2024 18:47
@st0012 st0012 force-pushed the drop-basic-object-workaround branch from a708d6a to f267df3 Compare December 18, 2024 18:57
@st0012
Copy link
Member Author

st0012 commented Dec 18, 2024

Update: Updating rubygems on CI resolves the issue.


Based on the debugging run's message:

Error: test_object_inspection_handles_basic_object(TestIRB::ContextTest): NameError: uninitialized constant PP::VERSION
/home/runner/work/irb/irb/test/irb/test_context.rb:161:in `test_object_inspection_handles_basic_object'
     15[8](https://github.com/ruby/irb/actions/runs/12399361528/job/34614111414?pr=1051#step:4:9):         irb.eval_input
     159:       end
     160:       assert_empty err
  => 161:       puts PP::VERSION
     162:       assert_not_match(/NoMethodError/, out)
     163:       assert_match(/#<BasicObject:.*>/, out)
     164:     ensure
Error: NameError: uninitialized constant PP::VERSION

It looks like Ruby 2.7's bundler failed to use the pp gem and still uses the one that's bundled with Ruby.

@st0012 st0012 force-pushed the drop-basic-object-workaround branch 2 times, most recently from a982abd to 7d99694 Compare January 11, 2025 14:26
`pp` 0.6.0+ includes ruby/pp#26 to handle BasicObject,
so we can drop the workaround.
@st0012 st0012 force-pushed the drop-basic-object-workaround branch from 7d99694 to 1dc176c Compare January 11, 2025 14:30
@st0012 st0012 marked this pull request as ready for review January 11, 2025 14:33
@@ -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.

@tompng tompng merged commit 08908d4 into master Jan 12, 2025
67 checks passed
@tompng tompng deleted the drop-basic-object-workaround branch January 12, 2025 12:47
matzbot pushed a commit to ruby/ruby that referenced this pull request Jan 12, 2025
(ruby/irb#1051)

`pp` 0.6.0+ includes ruby/pp#26 to handle BasicObject,
so we can drop the workaround.

ruby/irb@08908d43c7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants