Skip to content

Enhancements for ENV doc #2751

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

Merged
merged 1 commit into from
Dec 16, 2019
Merged

Enhancements for ENV doc #2751

merged 1 commit into from
Dec 16, 2019

Conversation

BurdetteLamar
Copy link
Member

No description provided.

Copy link
Member

@nobu nobu left a comment

Choose a reason for hiding this comment

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

Could you squash the removal of unused files?

hash.c Outdated
* Raises an exception if a name or value is invalid.
* See {Invalid Names and Values}[#class-ENV-label-Invalid-Names+and+Values].
*
* Adds new environment variables up to the first error encountered:
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we can/should define the result after an error.

Copy link
Member Author

Choose a reason for hiding this comment

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

For other errors, we say nothing about the state of ENV, implying that it's not changed (which is true). Here, the state of ENV -is- changed, and I think we have to say so.

Actually, I think this is a bug. On an error, ENV should never be changed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Demo:

irb
ENV.clear

=> {}

ENV.replace('foo' => '0', Object.new => Object.new)
Traceback (most recent call last):
5: from C:/Ruby26-x64/bin/irb.cmd:31:in <main>' 4: from C:/Ruby26-x64/bin/irb.cmd:31:in load'
3: from C:/Ruby26-x64/lib/ruby/gems/2.6.0/gems/irb-1.0.0/exe/irb:11:in <top (required)>' 2: from (irb):2 1: from (irb):2:in replace'
TypeError (no implicit conversion of Object into String)
ENV

=> {"foo"=>"0"}

Copy link
Member

Choose a reason for hiding this comment

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

Also ENV.update has same behavior, and I think it is hard to guarantee your expectation as the change would be expected immediately in its block form.

Copy link
Member Author

Choose a reason for hiding this comment

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

So, ready to merge now?

* - A String.
* - An object that responds to \#to_str by returning a String, in which case that String will be used as the name or value.
*
* ==== Invalid Names and Values
Copy link
Member

Choose a reason for hiding this comment

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

Could the common requirements to name and value be extracted?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure.

@BurdetteLamar BurdetteLamar marked this pull request as ready for review December 14, 2019 19:00
@matzbot matzbot merged commit d6fd390 into ruby:master Dec 16, 2019
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

Successfully merging this pull request may close these issues.

3 participants