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 parsing of ENV variable values to Boolean type #180

Merged
merged 1 commit into from
Oct 23, 2017
Merged

Add parsing of ENV variable values to Boolean type #180

merged 1 commit into from
Oct 23, 2017

Conversation

mjgiarlo
Copy link
Contributor

@mjgiarlo mjgiarlo commented Oct 19, 2017 •
edited
Loading

Fixes #178

(Discovered in samvera/hyku#1462. Heads-up, @tdonohue.)

@mjgiarlo mjgiarlo force-pushed the parse_boolean_env_values branch 2 times, most recently from 4ac4517 to d89ed7c Compare October 19, 2017 20:43
@mjgiarlo
Copy link
Contributor Author

@pkuczynski I can eliminate the CodeClimate issue by taking the logic from #__boolean and inlining it in #__value, but I didn't want to change the one-liner pattern going there. What do you advise? Should I make CodeClimate happy, or are there other changes you'd like to see?

tdonohue added a commit to samvera/hyku that referenced this pull request Oct 19, 2017
@tdonohue
Monkeypatch config gem per rubyconfig/config#180
64c697d
@tdonohue tdonohue mentioned this pull request Oct 19, 2017
Merged
mjgiarlo added a commit to samvera/hyku that referenced this pull request Oct 19, 2017
@mjgiarlo
Merge pull request #1463 from samvera-labs/config-monkeypatch
e2783a5 
Monkey-patch config gem per rubyconfig/config#180
@mjgiarlo mjgiarlo mentioned this pull request Oct 19, 2017
Closed
tdonohue added a commit to samvera/hyku that referenced this pull request Oct 20, 2017
@tdonohue
Update gemfile to point at rubyconfig/config#180 branch. Remove monke…
0becb4b 
…ypatch
@tdonohue tdonohue mentioned this pull request Oct 20, 2017
Merged
tdonohue added a commit to samvera/hyku that referenced this pull request Oct 20, 2017
@tdonohue
Update gemfile to point at rubyconfig/config#180 branch. Remove monke…
2b98718 
…ypatch
tdonohue added a commit to samvera/hyku that referenced this pull request Oct 20, 2017
@tdonohue
Merge pull request #1465 from samvera-labs/update_config_revert_monke…
5665392 
…ypatch

Update Gemfile to point at rubyconfig/config#180 branch for config gem
@tdonohue tdonohue mentioned this pull request Oct 20, 2017
Closed
@pkuczynski
Copy link
Member

pkuczynski commented Oct 21, 2017 •
edited
Loading

I would rather see this done as:

    # Try to convert string to a correct type
    def __value(v)
      case value
      when 'false'
        false
      when 'true'
        true
      else
        Integer(v) rescue Float(v) rescue v
      end
    end

@pkuczynski pkuczynski self-requested a review October 21, 2017 16:59
mjgiarlo
mjgiarlo commented Oct 23, 2017
View reviewed changes
Copy link
Contributor Author

@mjgiarlo mjgiarlo left a comment

Choose a reason for hiding this comment

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

Dig the modification you made. I believe there may be a typo in it.

lib/config/options.rb Outdated
@@ -188,7 +188,14 @@ def __convert(h) #:nodoc:

# Try to convert string to a correct type
def __value(v)
Integer(v) rescue Float(v) rescue v
case value
Copy link
Contributor Author

Choose a reason for hiding this comment

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

s/value/v/ here, right?

Copy link
Member

Choose a reason for hiding this comment

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

yeah, totally! sorry!

@mjgiarlo mjgiarlo force-pushed the parse_boolean_env_values branch from 0eb432e to 36bb485 Compare October 23, 2017 15:11
@mjgiarlo
Copy link
Contributor Author

@pkuczynski I fixed a typo in the change you submitted, squashed commits, and pushed again. Thanks for the change! I suspect this is now ready to go.

When do you think 1.6.0 can get released with this change?

@mjgiarlo mjgiarlo mentioned this pull request Oct 23, 2017
Closed
@pkuczynski pkuczynski merged commit a2ae5d7 into rubyconfig:master Oct 23, 2017
@pkuczynski
Copy link
Member

Done! Released as 1.5.1.

@pkuczynski
Copy link
Member

Thanks for your contributiuon :)

@mjgiarlo
Copy link
Contributor Author

Thanks for the useful gem, @pkuczynski!

@mjgiarlo mjgiarlo deleted the parse_boolean_env_values branch October 23, 2017 16:03
@pkuczynski pkuczynski added this to the 0.3.pre milestone Jan 8, 2020
alishaevn pushed a commit to rkuehn-uofl/hyku that referenced this pull request Apr 10, 2023
@tdonohue
Monkeypatch config gem per rubyconfig/config#180
ffd49bf
alishaevn pushed a commit to rkuehn-uofl/hyku that referenced this pull request Apr 10, 2023
@mjgiarlo
Merge pull request #1463 from samvera-labs/config-monkeypatch
7e1c3ae 
Monkey-patch config gem per rubyconfig/config#180
alishaevn pushed a commit to rkuehn-uofl/hyku that referenced this pull request Apr 10, 2023
@tdonohue
Update gemfile to point at rubyconfig/config#180 branch. Remove monke…
5644e30 
…ypatch
alishaevn pushed a commit to rkuehn-uofl/hyku that referenced this pull request Apr 10, 2023
@tdonohue
Merge pull request #1465 from samvera-labs/update_config_revert_monke…
0ab99a1 
…ypatch

Update Gemfile to point at rubyconfig/config#180 branch for config gem
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pkuczynski pkuczynski pkuczynski approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
0.3.pre
Development

Successfully merging this pull request may close these issues.

Support booleans with env_parse_values
2 participants
@mjgiarlo @pkuczynski