-
Notifications
You must be signed in to change notification settings - Fork 369
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
Support spaces in environment variable lists #2011
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -24,10 +24,34 @@ def env_to_float(var, default = nil) | |||||
var && ENV.key?(var) ? ENV[var].to_f : default | ||||||
end | ||||||
|
||||||
def env_to_list(var, default = []) | ||||||
# Parses comma- or space-separated lists. | ||||||
# | ||||||
# If a comma is present, then the list is considered comma-separated. | ||||||
# Otherwise, it is considered space-separated. | ||||||
# | ||||||
# After the entries are separated, commas and whitespaces that are | ||||||
# either trailing or leading are trimmed. | ||||||
# | ||||||
# Empty entries, after trimmed, are also removed from the result. | ||||||
def env_to_list(var, default = [], comma_separated_only: false) | ||||||
var = decode_array(var) | ||||||
if var && ENV.key?(var) | ||||||
ENV[var].split(',').map(&:strip) | ||||||
value = ENV[var] | ||||||
|
||||||
values = if value.include?(',') || comma_separated_only | ||||||
value.split(',') | ||||||
else | ||||||
value.split # space splitting is the default | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: In the spirit of self-documenting code,
Suggested change
is faster to read than reading the comment ;) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rubocop yelled at me for having the parameter that matches the default, so I removed it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahh Rubocop... That extremely opinionated colleague that we know nothing about :) |
||||||
end | ||||||
|
||||||
values.map! do |v| | ||||||
v.gsub!(/\A[\s,]*|[\s,]*\Z/, '') | ||||||
|
||||||
v.empty? ? nil : v | ||||||
end | ||||||
|
||||||
values.compact! | ||||||
values | ||||||
else | ||||||
default | ||||||
end | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -174,7 +174,7 @@ | |
end | ||
end | ||
|
||
context 'when env var is set as' do | ||
context 'when env var is set' do | ||
include_context 'env var' | ||
|
||
context '\'\'' do | ||
|
@@ -201,11 +201,45 @@ | |
it { is_expected.to eq(%w[1 2]) } | ||
end | ||
|
||
context ' 1 , 2 , 3 ' do | ||
let(:env_value) { ' 1 , 2 , 3 ' } | ||
context ' 1 , 2 , 3 ' do | ||
let(:env_value) { ' 1 , 2 , 3 ' } | ||
|
||
it { is_expected.to eq(%w[1 2 3]) } | ||
end | ||
|
||
context '1 2 3' do | ||
let(:env_value) { '1 2 3' } | ||
|
||
it { is_expected.to eq(%w[1 2 3]) } | ||
end | ||
|
||
context '1,2 3' do | ||
let(:env_value) { '1,2 3' } | ||
|
||
it { is_expected.to eq(['1', '2 3']) } | ||
end | ||
Comment on lines
+216
to
+220
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be honest, I'm not sure how useful it is to support this, rather than just doing split by space and comma at the same time and be done with it. But it is true that the Python implementation also does some of these hijinks so... (We're not actually matching the exact hijinks with spaces in python, e.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm following Go's implementation, Java's implementation, and Python's original implementation document (internal doc):
I'm trying to match their implementations, to keep consistency, instead of doing what seems more right here. |
||
|
||
context ' 1 2 3 ' do | ||
let(:env_value) { ' 1 2 3 ' } | ||
|
||
it { is_expected.to eq(%w[1 2 3]) } | ||
end | ||
|
||
context '1,, ,2,3,' do | ||
let(:env_value) { '1,, ,2,3,' } | ||
|
||
it { is_expected.to eq(%w[1 2 3]) } | ||
end | ||
|
||
context 'and comma_separated_only is set' do | ||
subject(:env_to_list) { variable_helpers.env_to_list(var, comma_separated_only: true) } | ||
|
||
context 'B3 single header ' do | ||
let(:env_value) { 'B3 single header ' } | ||
|
||
it { is_expected.to eq(['B3 single header']) } | ||
end | ||
end | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: Would it perhaps make sense to make this a required argument, to make callers explicitly think about if they need commas or spaces for separation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.