Hash inspection not working as in MRI for sprintf %p #2161

Closed
headius opened this Issue Nov 9, 2014 · 5 comments

Comments

Projects
None yet
5 participants
@headius
Member

headius commented Nov 9, 2014

The test that's failing in mri/ruby/test_sprintf.rb:

  def test_hash
    options = {:capture=>/\d+/}
    assert_equal("with options {:capture=>/\\d+/}", sprintf("with options %p" % options))
  end

The failure:

ArgumentError: positional args mixed with named args
    org/jruby/RubyString.java:1221:in `%'
    /Users/headius/projects/jruby/test/mri/ruby/test_sprintf.rb:173:in `test_hash'

We do have code that raises this error around Sprintf.java:154 in Sprintf::Args#next, but I was not able to figure out the equivalent code in MRI (it doesn't seem to be there in the GETNEXTARG macro in sprintf.c nor in the logic for case 'p' in the main switch) nor does the test itself indicate where/when the change to sprintf was actually made.

@headius headius added the core label Nov 9, 2014

@headius headius added this to the JRuby 9.0.0.0 milestone Nov 9, 2014

@enebo enebo added the JRuby 9000 label Jul 14, 2015

@enebo enebo modified the milestone: JRuby 9.0.0.0 Jul 14, 2015

@uchagani

This comment has been minimized.

Show comment
Hide comment
@uchagani

uchagani Mar 29, 2016

This is still failing as of jruby 9.0.5.0 (2.2.3).

This is still failing as of jruby 9.0.5.0 (2.2.3).

@denyago

This comment has been minimized.

Show comment
Hide comment
@denyago

denyago Feb 3, 2017

This is still failing as of jruby 9.1.7.0 (2.3.1) 😿

denyago commented Feb 3, 2017

This is still failing as of jruby 9.1.7.0 (2.3.1) 😿

@enebo enebo closed this in 86a1058 Feb 10, 2017

@enebo enebo added this to the JRuby 9.1.8.0 milestone Feb 10, 2017

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Feb 10, 2017

Member

Added test to spec in e66f638 ... I think @eregon will still merge this back upstream?

Member

enebo commented Feb 10, 2017

Added test to spec in e66f638 ... I think @eregon will still merge this back upstream?

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Feb 13, 2017

Member

@enebo That's a good question. @eregon: Do we need to take over upstreaming our ruby/spec changes?

Member

headius commented Feb 13, 2017

@enebo That's a good question. @eregon: Do we need to take over upstreaming our ruby/spec changes?

@eregon

This comment has been minimized.

Show comment
Hide comment
@eregon

eregon Feb 14, 2017

Member

@enebo @headius I can keep doing it, no worries.
It's convenient since I merge from all repos and then update them all.
I did the last merge about 2 weeks ago.
I might ask your help for tagging new failures if it's not trivial though :)

Member

eregon commented Feb 14, 2017

@enebo @headius I can keep doing it, no worries.
It's convenient since I merge from all repos and then update them all.
I did the last merge about 2 weeks ago.
I might ask your help for tagging new failures if it's not trivial though :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment