Fix EasyConfig.update code to handle both strings and lists as input.#3170
Conversation
Correctly handle allow_duplicate=False when "value" is a partial match for an item in key when key is a string.
…around added item to match old behaviour.
|
This fixes the build problem for VMD described in easybuilders/easybuild-easyblocks#1755 |
| msg += "attempted update value, '%s', is not a string or list." | ||
| raise EasyBuildError(msg, key, value) | ||
|
|
||
| prev_value = self[key] |
There was a problem hiding this comment.
prev_value should be renamed now that we're updating it in place?
There was a problem hiding this comment.
any suggestions? new_value?
| self[key] = prev_value + [item] | ||
| for item in lval: | ||
| if allow_duplicate or item not in prev_value: | ||
| prev_value.append(item) |
There was a problem hiding this comment.
You'll need to be careful here... prev_value is a reference to the original value, .append will append to the original value, whereas before we were creating a new value using prev_value + [item]. That was not by accident probably...
There was a problem hiding this comment.
Ah, can never remember the reference thingie...
There was a problem hiding this comment.
But it doesn't really matter, we could use self[key] directly, since the checks should be performed on the whole string/list each time.
I.e. get rid of prev_value altogether.
There was a problem hiding this comment.
We were not changing the existing value in self[key] inline, but we were constructing a new value and assigning self[key] to that value. You should keep that behavior, or we may run into surprises.
Fictic example in an easyblock:
alist = ['--foo']
self.cfg['configopts'] = alist
self.cfg.update('configopts', '--bar')
for x in alist:
...The contents of alist should not change (self.cfg.update should not have side effects)
There was a problem hiding this comment.
ah, those pescy references...
| if allow_duplicate or value not in prev_value: | ||
| self[key] = '%s %s ' % (prev_value, value) | ||
| for item in lval: | ||
| if allow_duplicate or (not re.search(r'(^|\s+)%s(\s+|$)' % re.escape(item), prev_value)): |
There was a problem hiding this comment.
I think the re.search deserves a clarifying comment above? Something like:
"# only add value to string if it's not there yet (surrounded by whitespace)
Rename temp variable to better match usage.
Correctly handle allow_duplicate=False when "value" is a partial match
for an item in key when key is a string.