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

encoding parameter for Dir.open, Dir.new #3205 #4495 #5915

Merged
merged 2 commits into from Oct 27, 2019

Conversation

@MariuszCwikla
Copy link
Contributor

MariuszCwikla commented Oct 14, 2019

Fixes #4495 and partial fix for #3205

@MariuszCwikla

This comment has been minimized.

Copy link
Contributor Author

MariuszCwikla commented Oct 15, 2019

In f5b78ad I've added fix for foreach and more tests

@kares

This comment has been minimized.

Copy link
Member

kares commented Oct 24, 2019

looking good, still could we maybe have : Dir.mkdir("./testDir_1") in a test setup
and also do a test teardown part to cleanup all of the created dir's content and remove it.

@kares kares added this to the JRuby 9.2.9.0 milestone Oct 24, 2019
@MariuszCwikla

This comment has been minimized.

Copy link
Contributor Author

MariuszCwikla commented Oct 25, 2019

@kares thanks for hints. Normally I would do like you suggested, but teardown already performs cleanup this in it's own "interesting" ;) way:

  • setup deletes testDir_1 up to testDir_5
  • teardown calls setup ;)

Do you agree to do more refactoring (including setup/teardown and other existing tests not related to my change?)

@kares

This comment has been minimized.

Copy link
Member

kares commented Oct 25, 2019

@MariuszCwikla oh I did not check, in that case all 👍 from me

@headius

This comment has been minimized.

Copy link
Member

headius commented Oct 27, 2019

The modified open and initialize could be split into one- and two-argument versions, which will usually avoid the args[] boxing, but it's a minor item. I'll make the fix quickly on master since we'd like this in 9.2.9.

@headius headius merged commit 8a75d31 into jruby:master Oct 27, 2019
5 checks passed
5 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jruby.jruby Build #20191015.4 succeeded
Details
jruby.jruby (Job linux) Job linux succeeded
Details
jruby.jruby (Job mac) Job mac succeeded
Details
jruby.jruby (Job windows) Job windows succeeded
Details
@headius

This comment has been minimized.

Copy link
Member

headius commented Oct 27, 2019

I notice that one particular piece of logic seems to be lost: in foreach, if an Encoding was passed in instead of an options hash, it would be used without trying to dig it out of options.

Was that behavior in error? It appears to have been there since 1.9.

I will commit a change to remove the [] bits, but we do need confirmation about that logic.

headius added a commit that referenced this pull request Oct 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.