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

Dir.foreach, Dir.new, Dir.open: Missing/incompatible encoding support. #3205

Closed
Freaky opened this issue Aug 1, 2015 · 8 comments
Closed

Comments

@Freaky
Copy link
Contributor

Freaky commented Aug 1, 2015

Ruby 2.2.2:

 Dir.new("/var/empty", encoding: Encoding.find("filesystem")).to_a # => [".", ".."]
 Dir.open("/var/empty", encoding: Encoding.find("filesystem")).to_a # => [".", ".."]
 Dir.foreach("/var/empty", encoding: Encoding.find("filesystem")).to_a # => [".", ".."]

JRuby 9.0.0.0 yields, respectively:

ArgumentError: wrong number of arguments calling `initialize` (2 for 1)
ArgumentError: wrong number of arguments calling `open` (2 for 1)
TypeError: wrong argument type Hash (expected Encoding)

The following works in JRuby:

Dir.foreach("/var/empty", Encoding.find("filesystem")).to_a # => [".", ".."]
Dir.new("/var/empty").each(Encoding.find("filesystem")).to_a # => [".", ".."]

But in Ruby 2.2.2 these raise:

ArgumentError: wrong number of arguments (2 for 1)
ArgumentError: wrong number of arguments (1 for 0)

It would seem the resolution to #2547 in 26ed114 is incorrect.

@MariuszCwikla
Copy link
Contributor

I would like to help on this. I did already some initial investigation and coding.
This also look like a duplicate of issue #4495, isn't it?

@MariuszCwikla
Copy link
Contributor

I prepared initial fix but I have some observations and questions.

  1. Dir.open, Dir.new, Dir.foreach support encoding both as string and as Encoding in CRuby:
p Dir.open('.', encoding: 'UTF-8').entries
p Dir.open('.', encoding: Encoding.find('UTF-8')).entries
p Dir.new('.', encoding: 'UTF-8').entries
p Dir.new('.', encoding: Encoding.find('UTF-8')).entries
Dir.foreach('.', encoding: 'UTF-8'){|f| p f}
Dir.foreach('.', encoding: Encoding.find('UTF-8')){|f| p f}
  1. I found MRI test test_dir_enc and implemented changes to make the test pass (i.e. encoding as String).
    I didn't find any MRI test with Encoding type.
    I would like to add them, but I'm not sure where to add. Should this be test_dir.rb under test/mri or under test/jruby?
    If test.mri then shouldn't this be contributed to MRI repo? (I believe I've seen such recommendation somewhere, but can't find it again)

What's the recommended approach in this case?

  1. Dir.foreach supports second parameter already in JRuby, but I believe it's not correctly implemented (bug). Second parameter should be "keyword argument", not regular parameter, i.e.
Dir.foreach('.', Encoding.find('UTF-8')){|f| p f}
  • CRuby (2.6.4): fails with ArgumentError (wrong number of arguments (given 2, expected 1))
  • JRuby: runs fine (i.e. bug)

correct syntax is Dir.foreach('.', encoding: 'UTF-8')

I would like to continue with:

  • fix foreach to use keyword arguments
  • support both string and Encoding in all above methods
  • implement remaining tests, but I need some guidance (until then I will prepare tests under test/jruby)

headius added a commit that referenced this issue Oct 27, 2019
encoding parameter for Dir.open, Dir.new #3205 #4495
@headius
Copy link
Member

headius commented Oct 27, 2019

@MariuszCwikla Thanks for your help with this and #4495 (now fixed by #5915)! Your plan going forward looks fine to me, and I am standing by to offer any assistance I can.

@Freaky
Copy link
Contributor Author

Freaky commented Feb 3, 2021

Just ran back into this with Dir.each_child - guess that one has been missed given entries, foreach, and children are all fine.

headius added a commit to headius/jruby that referenced this issue Feb 3, 2021
@headius
Copy link
Member

headius commented Feb 3, 2021

I have pushed #6551 to try to unify this encoding logic and bring these methods into harmony. The available tests are no greener than they were before, but test/mri/ruby/test_dir_m17n runs now (was failing to load) and could be iterated on.

@headius
Copy link
Member

headius commented Feb 3, 2021

@MariuszCwikla Dunno if you are still interested in helping here but going through the test_dir_m17n failures one by one might be fun! 😀

@headius headius added this to the JRuby 9.2.15.0 milestone Feb 3, 2021
Freaky added a commit to Freaky/fast_find that referenced this issue Feb 4, 2021
@enebo enebo modified the milestones: JRuby 9.2.15.0, JRuby 9.2.16.0 Feb 23, 2021
@enebo
Copy link
Member

enebo commented Feb 23, 2021

This is not ready and .15 will be coming out very soon.

headius added a commit that referenced this issue Feb 23, 2021
@headius
Copy link
Member

headius commented Feb 23, 2021

All original cases now work properly after #6551 and subsequent commits 04f0c82 and 937792b. Specs were added for the kwarg use cases but not for the error case that we used to allow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants