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.

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

Don't save kebab-case aliases in the params map #4702

Merged
merged 6 commits into from
Sep 30, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions modules/nextflow/src/main/groovy/nextflow/cli/CmdRun.groovy
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ import nextflow.plugin.Plugins
import nextflow.scm.AssetManager
import nextflow.script.ScriptFile
import nextflow.script.ScriptRunner
import nextflow.secret.SecretsLoader
import nextflow.util.AliasMap
import nextflow.util.CustomPoolFactory
import nextflow.util.Duration
import nextflow.util.HistoryFile
Expand Down Expand Up @@ -619,7 +621,7 @@ class CmdRun extends CmdBase implements HubOptions {
@Memoized // <-- avoid parse multiple times the same file and params
Map parsedParams(Map configVars) {

final result = [:]
final result = new AliasMap()
final file = getParamsFile()
if( file ) {
def path = validateParamsFile(file)
Expand All @@ -634,7 +636,7 @@ class CmdRun extends CmdBase implements HubOptions {
if( !params )
return result

for( Map.Entry<String,String> entry : params ) {
for( def entry : params ) {
addParam( result, entry.key, entry.value )
}
return result
Expand Down
165 changes: 17 additions & 148 deletions modules/nextflow/src/main/groovy/nextflow/script/ScriptBinding.groovy
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import groovy.util.logging.Slf4j
import nextflow.NF
import nextflow.Session
import nextflow.exception.AbortOperationException
import org.apache.commons.lang.StringUtils
/**
* Defines the script execution context. By default provided the following variables
* <li>{@code __$session}: the current execution session
Expand Down Expand Up @@ -202,23 +201,17 @@ class ScriptBinding extends WorkflowBinding {
}

/**
* Holds parameter immutable values
* Implements immutable params map
*/
@CompileStatic
static class ParamsMap implements Map<String,Object> {

private Set<String> readOnlyNames

private List<String> realNames

private List<String> scriptAssignment = []

@Delegate
private Map<String,Object> target

ParamsMap() {
readOnlyNames = []
realNames = []
target = new LinkedHashMap<>()
}

Expand All @@ -227,23 +220,15 @@ class ScriptBinding extends WorkflowBinding {
putAll(values)
}

private ParamsMap(ParamsMap template, Map overrides) {
this(template)
allowNames(overrides.keySet())
putAll(overrides)
}
private ParamsMap(ParamsMap values, Map<String,?> overrides) {
this(values)

ParamsMap copyWith(Map overrides) {
return new ParamsMap(this, overrides)
for( def entry : overrides )
put0(entry.key, entry.value, true)
}

private ParamsMap allowNames(Set names) {
for( String name : names ) {
final name2 = name.contains('-') ? hyphenToCamelCase(name) : camelCaseToHyphen(name)
readOnlyNames.remove(name)
readOnlyNames.remove(name2)
}
return this
ParamsMap copyWith(Map<String,?> overrides) {
return new ParamsMap(this, overrides)
}

@Override
Expand All @@ -269,142 +254,26 @@ class ScriptBinding extends WorkflowBinding {
@Override
String put(String name, Object value) {
assert name
(name in scriptAssignment
? log.warn("`params.$name` is defined multiple times -- Assignments following the first are ignored")
: scriptAssignment << name )
if( name in scriptAssignment )
log.warn("`params.$name` is defined multiple times -- Assignments following the first are ignored")
else
scriptAssignment << name
return put0(name,value)
}

private String put0(String name, Object value) {

// keep track of the real name
realNames << name

// normalize the name
def name2 = name.contains('-') ? hyphenToCamelCase(name) : camelCaseToHyphen(name)

final readOnly = name in readOnlyNames || name2 in readOnlyNames

def result = null
if( !readOnly ) {
readOnlyNames << name
readOnlyNames << name2
result = target.put(name, value)
target.put(name2, value)
}

return result
private String put0(String name, Object value, boolean allowOverride=false) {
return allowOverride || !target.containsKey(name)
? target.put(name, value)
: null
}

@Override
void putAll(Map other) {
for( Entry<String,Object> entry : other.entrySet() ) {
void putAll(Map<String,Object> other) {
for( def entry : other.entrySet() ) {
put0(entry.getKey(), entry.getValue())
}
}

/**
* Renders a string containing all name-value pairs as a command line string
*
* @param opts String formatting options:
* {@code sep}: the key-value pair separator
* {@code quote}: the character to be used to quote the parameter value
* {@code prefix}: the character(s) prepended to the parameter key
*
* @return A command line formatted string e.g. {@code --foo x --bar y}
*/
String all(Map opts = null) {

String sep = opts?.sep ?: ' '
String quote = opts?.quote ?: ''
String prefix = opts?.prefix ?: '--'

def result = []
realNames.each {
result << ("$prefix$it$sep${wrap(this.get(it).toString(), quote)}".toString())
}
return result.join(' ')
}

/**
* Wrap a string value with quote characters
*
* @param str The string with value to be wrapped by quote characters
* @param quote The quote character
* @return The quoted string
*/
@PackageScope
static String wrap( String str, String quote ) {
if( !quote && (str.contains(' ') || str.contains('"') || str.contains("'") ))
quote = '"'

if( !quote )
return str

def result = new StringBuilder()
result.append(quote)
str.each {
result.append( it == quote ? '\\'+quote : it )
}
result.append(quote)
return result.toString()
}

/**
* Converts a string containing words separated by a hyphen character to a camelCase string
*
* @param str The string to be converted
* @return
*/
@PackageScope
static String hyphenToCamelCase( String str ) {

if( !str ) { return str }

def result = new StringBuilder()
str.split('-').eachWithIndex{ String entry, int i ->
result << (i>0 ? StringUtils.capitalize(entry) : entry )
}

return result.toString()
}

/**
* Converts a camel-case string to a string where words are separated by hyphen character
*
* @param str The string to be converted
* @return A string where camel-case words are converted to words separated by hyphen character
*/
@PackageScope
static String camelCaseToHyphen( String str ) {

def lower = 'a'..'z'
def upper = 'A'..'Z'

def result = new StringBuilder()
if( !str ) {
return str
}

result << str[0]
for( int i=1; i<str.size(); i++ ) {
if( str[i] in upper && str[i-1] in lower ) {
result << '-'
if( i+1<str.size() && str[i+1] in lower ) {
result << str[i].toLowerCase()
}
else {
result << str[i]
}
}
else {
result << str[i]
}
}

return result.toString()
}

}

}
Original file line number Diff line number Diff line change
Expand Up @@ -75,65 +75,30 @@ class ScriptBindingTest extends Specification {

}

def 'should convert hyphen separated string to camel case' () {

expect:
ScriptBinding.ParamsMap.hyphenToCamelCase('a') == 'a'
ScriptBinding.ParamsMap.hyphenToCamelCase('A') == 'A'
ScriptBinding.ParamsMap.hyphenToCamelCase('a-b-c-') == 'aBC'
ScriptBinding.ParamsMap.hyphenToCamelCase('aa-bb-cc') == 'aaBbCc'
ScriptBinding.ParamsMap.hyphenToCamelCase('alpha-beta-delta') == 'alphaBetaDelta'
ScriptBinding.ParamsMap.hyphenToCamelCase('Alpha-Beta-delta') == 'AlphaBetaDelta'

}

def 'should convert camel case string to hyphen separated' () {

expect:
ScriptBinding.ParamsMap.camelCaseToHyphen('alphaBetaDelta') == 'alpha-beta-delta'
ScriptBinding.ParamsMap.camelCaseToHyphen('AlphaBetaDelta') == 'Alpha-beta-delta'
ScriptBinding.ParamsMap.camelCaseToHyphen('Field1') == 'Field1'
ScriptBinding.ParamsMap.camelCaseToHyphen('FieldUno') == 'Field-uno'
ScriptBinding.ParamsMap.camelCaseToHyphen('FieldUNO') == 'Field-UNO'
ScriptBinding.ParamsMap.camelCaseToHyphen('FieldA') == 'Field-A'
ScriptBinding.ParamsMap.camelCaseToHyphen('FieldAB') == 'Field-AB'
ScriptBinding.ParamsMap.camelCaseToHyphen('FieldAb') == 'Field-ab'

}

def 'should put an entry in the params map' () {

when:
def map = new ScriptBinding.ParamsMap()
map['alphaBeta'] = 1
map['alphaBeta'] = 2
map['alpha-beta'] = 3

then:
map['alphaBeta'] == 1
map['alpha-beta'] == 1


when:
map = new ScriptBinding.ParamsMap()
map['aaa-bbb-ccc'] = 1
map['aaaBbbCcc'] = 10
map['AaaBbbCcc'] = 20

then:
map['aaaBbbCcc'] == 1
map['aaa-bbb-ccc'] == 1

map['aaaBbbCcc'] == 10
map['AaaBbbCcc'] == 20

when:
map = new ScriptBinding.ParamsMap()
map['field1'] = 1
map['field2'] = 2
map['Field2'] = 3

then:
map['field1'] == 1
map['field-1'] == null
map['field2'] == 2
map['Field2'] == 3

Expand All @@ -151,7 +116,6 @@ class ScriptBindingTest extends Specification {
then:
map.alpha == 0
map.alphaBeta == 0
map.'alpha-beta' == 0
map.delta == 2
map.gamma == 3

Expand All @@ -160,68 +124,17 @@ class ScriptBindingTest extends Specification {
then:
copy.foo == 1
copy.alphaBeta == 4
copy.'alpha-beta' == 4
copy.delta == 2
copy.gamma == 3
copy.omega == 9
and:
// source does not change
map.alpha == 0
map.alphaBeta == 0
map.'alpha-beta' == 0
map.delta == 2
map.gamma == 3
!map.containsKey('omega')

}

def 'should wrap a string value with quote character' () {

expect:
ScriptBinding.ParamsMap.wrap('hello',null) == 'hello'
ScriptBinding.ParamsMap.wrap('hello','"') == '"hello"'
ScriptBinding.ParamsMap.wrap('hello world',null) == '"hello world"'
ScriptBinding.ParamsMap.wrap('hello world',"'") == "'hello world'"
ScriptBinding.ParamsMap.wrap('hello"world',"'") == "'hello\"world'"
ScriptBinding.ParamsMap.wrap('hello"world',null) == '"hello\\"world"'

}

def 'should return a command line formatted string'() {

when:
def params = new ScriptBinding.ParamsMap('foo-bar':1)
then:
params.size() == 2
params.fooBar == 1
params.'foo-bar' == 1
params.all() == '--foo-bar 1'

expect:
new ScriptBinding.ParamsMap(x:1).all() == '--x 1'
new ScriptBinding.ParamsMap(x:1, y: 2).all() == '--x 1 --y 2'
new ScriptBinding.ParamsMap(x:1, y: 2).all(sep:'=') == '--x=1 --y=2'
new ScriptBinding.ParamsMap(x:1, y: 2).all(sep:'=', prefix:'-') == '-x=1 -y=2'
new ScriptBinding.ParamsMap(x:1, y: 'hello world').all() == '--x 1 --y "hello world"'
new ScriptBinding.ParamsMap(x:1, y: 'hello world').all(quote:"'") == '--x \'1\' --y \'hello world\''
new ScriptBinding.ParamsMap(x:1, y: "O'Connors").all(quote:"'") == "--x '1' --y 'O\\'Connors'"

}

def 'should get the variable name giving the value'() {

given:
def X=1
def Y='hello'
def binding = new ScriptBinding([:])
binding.setVariable('foo', X)
binding.setVariable('bar', Y)

expect:
binding.getVariableName(X) == 'foo'
binding.getVariableName(Y) == 'bar'
binding.getVariableName(new String(Y)) == null
binding.getVariableName(null) == null
pditommaso marked this conversation as resolved.
Show resolved Hide resolved
}

}
Loading