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.

Sign up for GitHub

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

Jump to bottom

Add slice method to ENV like Hash#slice #1844

Closed
wants to merge 1 commit into from
Closed

Add slice method to ENV like Hash#slice #1844

wants to merge 1 commit into from

Conversation

benoittgt
Copy link
Contributor

@benoittgt benoittgt commented Mar 21, 2018 •
edited
Loading

Hello

After reading https://bugs.ruby-lang.org/issues/14559 I thought it could be a good idea to try to implement this during the RubyHackChallenge at Cookpad office in Bristol.

Here is my first PR for MRI, feel free to make any comments.
I don't know if I need to add entry changelog somewhere.

Thanks @mame for the help when I was working on.

@benoittgt benoittgt mentioned this pull request Mar 21, 2018
Closed
@nobu
Copy link
Member

nobu commented Mar 21, 2018

Could you adjust the indent?

@benoittgt
Copy link
Contributor Author

Sorry. Is it better now @nobu ?

Thanks for the review

@benoittgt
Copy link
Contributor Author

Hello @nobu
Sorry to ping you again.

nobu
nobu reviewed Mar 29, 2018
View reviewed changes
test/ruby/test_env.rb Outdated
ENV["foo"] = "bar"
ENV["baz"] = "qux"
ENV["bar"] = "rab"
assert_equal(ENV.slice(), {})
Copy link
Member

Choose a reason for hiding this comment

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

The order of assert_equal is (expected, result).

@nobu
Copy link
Member

nobu commented Mar 29, 2018

Seems fine, except for the arguments order of assertions.
I think that this would be acceptable.

@benoittgt
Copy link
Contributor Author

benoittgt commented Mar 29, 2018 •
edited
Loading

Sorry for the arguments order of assertions @nobu. It should be good now.

I have one failing test on AppVeyor but it seems unrelated

# Running tests:
running file: C:/projects/ruby/test/open-uri/test_open-uri.rb
Some worker was crashed. It seems ruby interpreter's bug
or, a bug of test/unit/parallel.rb. try again without -j
option.
NMAKE : fatal error U1077: '.\ruby.exe' : return code '0x1'
Stop.
Command exited with code 2

@benoittgt benoittgt closed this Apr 1, 2018
@benoittgt benoittgt reopened this Apr 1, 2018
@benoittgt
Copy link
Contributor Author

CI fixed. @nobu do I need to do something else to have this merged?

@benoittgt
Copy link
Contributor Author

Closed with 9760a7f

Thanks @nobu

@benoittgt benoittgt closed this Apr 23, 2018
@benoittgt benoittgt deleted the add_env_slice branch April 23, 2018 12:09
@hsbt hsbt added the Backport label Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nobu nobu nobu left review comments

Assignees
No one assigned
Labels
Backport
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@benoittgt @nobu @hsbt